Update Icons to v1.11.3 #42
Conversation
examples/icons/index.html
Outdated
| @@ -4,7 +4,7 @@ | |||
| <base data-trunk-public-url/> | |||
| <meta charset="utf-8"/> | |||
| <title>Yew App</title> | |||
| <link rel="stylesheet" href="bootstrap-icons-v1.10.5/bootstrap-icons.css"/> | |||
| <link rel="stylesheet" href="bootstrap-icons-v1.11.3/bootstrap-icons.css"/> | |||
There was a problem hiding this comment.
Perhaps we should not put the version in the directory; it is already in the header of the file.
There was a problem hiding this comment.
I disagree with myself now re: caching
| @@ -1,14 +1,14 @@ | |||
| /*! | |||
| * Bootstrap Icons v1.10.5 (https://icons.getbootstrap.com/) | |||
| * Copyright 2019-2023 The Bootstrap Authors | |||
| * Bootstrap Icons v1.11.3 (https://icons.getbootstrap.com/) | |||
There was a problem hiding this comment.
The version is identified here, so no need to identify it in the directory and complicate things.
e: I'm wrong; there's value for caching systems
|
I agree that it's not a good idea to manually change the version in the path when upgrading this package. The downside of removing the version from the path I see is that trunk does not apply the hash-renaming and thus a new version would be at the same path and a browser might think it's version is still accurate and thus not load it again. I've added a commit which generates the link automatically. If you prefer to remove the version from the path, I'll do that. |
|
I've updated the branch with better code.
|
There was a problem hiding this comment.
Appologizes, I never submitted this review. I didn't realize that the comments were invisible until I did so.
Double apologies in that this review is probably annoying for you, especially after this delay. Generalizing this solution seems to be a problem that others are having, as mentioned above.
| for l in 0..5 { | ||
| let mut path_base = PathBuf::from( | ||
| std::env::var("TRUNK_SOURCE_DIR").expect("Environment variable TRUNK_SOURCE_DIR"), | ||
| ); | ||
| for _ in 0..l { | ||
| path_base = path_base.join(".."); | ||
| } |
There was a problem hiding this comment.
I'm having trouble understanding what this looping is for, could you please explain it?
|
|
||
| fn main() -> Result<(), std::io::Error> { | ||
| BIFiles::generate_link_bootstrap_icons(&PathBuf::from( | ||
| std::env::var("TRUNK_SOURCE_DIR").expect("Environment variable TRUNK_SOURCE_DIR"), |
There was a problem hiding this comment.
Looks like this environment variable gets automatically set when running with trunk, cool beans. We might be screwing over people that use wasm-pack or something.
| for l in 0..5 { | ||
| let mut path = std::path::PathBuf::from(base_path); | ||
| for _ in 0..l { | ||
| path.push(".."); | ||
| } |
There was a problem hiding this comment.
I think this looping is going a level deeper until it finds the target directory. I don't love this experimental approach, crawling the user's file system. It won't always work either: for example, on my systems I use this in my ~/.cargo/config:
[build]
target-dir = "/home/matt/.cargo/build"
Meaning that my target directory is always under that shared directory, regardless of where I compile something. This build script will never find it.
Looks like this is a common problem: rust-lang/cargo#9661
|
No worries about the delay, though I was beginning to wonder. Good point about the target folder, and I didn't even know it existed. Unfortunately there is no option in trunk to include the output of a command into the html. I think this means that I should remove the last commit (the move this into the lib). And leaves us with a few options:
|
|
How about we include the generated file with each release, and rather than make a build script for generating these files we instead have a github action for it. We can control the environment of the runner for that action (i.e.: we can ensure the target directory will be found). The action will reside in our source tree, so folks can steal it if they want to DIY the generation. I agree with your concerns around caching; it's probably best to have the version in the filename. |
Oh, that sounds like an good idea. Though I'm not good at github actions, maybe you can write it? I've removed the commits with my try from the PR. |
I can try; I'm no expert at it either; I've implemented them a couple of times with instructions. |
|
I was thinking about another problem and found another solution for this problem. I've added it to this branch: https://github.com/alexkazik/yew-bootstrap/commits/update-icons2/ |
This comment was marked as outdated.
This comment was marked as outdated.
|
That's really weird. btw. I now also moved my target directory to a global space. |
|
My bad; my browser settings were set to ignore font selections other than what I had configured. Your new approach appears to work well! |
|
Good to hear. I would force push the new approach to this MR if that's ok. |
Go for it; I'll kill the existing branch on my end to avoid a mess. |
|
Done. And I'm satisfied with this solution. (Btw. I also tried to reduce the number of version numbers, but tried to use const str but that can't be used with concat, good idea with the macro.) |
Great! I'll do a final check locally and then merge it. Thanks for your contribution, you've improved the way we handle icons.
It seemed like a maintainance nightmare, it had to die! I started with a |
|
Glad to help - until next time (which should now be easier ;-) |
Updates the included/referenced bootstrap-icons version.
Please be aware that this is a breaking change for the users if the cdn is not used (as there is a version number in the path to the file). (I have an idea to avoid this in the future, I'll update the PR later.)
Changes to the generated icons: