Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed all icons for svgs #554

Merged

Conversation

SergioCasCeb
Copy link
Contributor

  • Previously all icons were imported from Font Awesome, so to not depend on such a system anymore all icons have been substituted for SVGS

- Changed the quick use button to a simpler, more obvious variation.
- Changed the quick use button to a simpler, more obvious variation.
The console now auto expands when the console is minimized and the user click on one of the console options.
- Previously all icon were been imported from Font Awesome, so in order to not depend on such system anymore all icons have been substituted for svgs
Copy link

netlify bot commented Jan 5, 2024

Deploy Preview for thingweb-playground ready!

Name Link
🔨 Latest commit d45f2f3
🔍 Latest deploy log https://app.netlify.com/sites/thingweb-playground/deploys/65aa9ade721e130008f103f4
😎 Deploy Preview https://deploy-preview-554--thingweb-playground.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@egekorkan egekorkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some SVGs that are contributed as "code". It would be better to have them all as .svg files

…ing-console-sergio

Auto-expanding the console when clicking on it
@egekorkan egekorkan linked an issue Jan 5, 2024 that may be closed by this pull request
@SergioCasCeb
Copy link
Contributor Author

SergioCasCeb commented Jan 5, 2024

@egekorkan the thing about having the icons as code, it allows me to change their color dynamically much easier. If I have them as images I have to utilize an extra mask on top of the image in order to change the image's colors.

@egekorkan
Copy link
Member

I can see the reasoning. In that case, we need to have a way to manage them. How do you generate the code? If it is simply by "exporting" from something like inkscape, we should still put the SVG files but not use them in the code but refer to them from the code via comments. Not needing this would be cooler though.

@SergioCasCeb
Copy link
Contributor Author

SergioCasCeb commented Jan 5, 2024

I am getting them from Font Awesome as they also allow you to download the SVGs or copy the code directly. That way the style stays consistent.

SergioCasCeb and others added 12 commits January 10, 2024 03:39
- The button text has been changed from Apply to Load to match the text of the quick use button
- Changed the quick use button to a simpler, more obvious variation.
- Changed the quick use button to a simpler, more obvious variation.
- The button text has been changed from Apply to Load to match the text of the quick use button
- Previously all icon were been imported from Font Awesome, so in order to not depend on such system anymore all icons have been substituted for svgs
- Rebased the branch to fix the merging conflicts added by the new console functionality
…r-button-sergio

Modified quick use button
@egekorkan
Copy link
Member

Some small things to do:

- Previously all icon were been imported from Font Awesome, so in order to not depend on such system anymore all icons have been substituted for svgs
- Previously all icon were been imported from Font Awesome, so in order to not depend on such system anymore all icons have been substituted for svgs
- Rebased the branch to fix the merging conflicts added by the new console functionality
- Previously all icon were been imported from Font Awesome, so in order to not depend on such system anymore all icons have been substituted for svgs
- Previously all icon were been imported from Font Awesome, so in order to not depend on such system anymore all icons have been substituted for svgs
- Added all missing attribuutions to svg (html and javascript)
- Fixed all merging issues
- Fixed style issues due to the merging and the new svgs
@SergioCasCeb
Copy link
Contributor Author

@egekorkan , I just realized now that it is deployed in Netlify, that when we build the project for production all comments are removed, including those in HTML for the SVG attribution, will this be an issue?

If it is we will probably will have to either:

  • Host the icons ourselves from Font awesome
  • use them all as images instead of inserting the code directly

@egekorkan
Copy link
Member

all comments are removed, including those in HTML for the SVG attribution, will this be an issue?

Nope, the source code matters as long as the source code is public :)

@egekorkan egekorkan linked an issue Jan 18, 2024 that may be closed by this pull request
@egekorkan
Copy link
Member

It is much better now. However, it is still not clear where the inline svgs are copied from so it is difficult to maintain later on.

@SergioCasCeb
Copy link
Contributor Author

It is much better now. However, it is still not clear where the inline svgs are copied from so it is difficult to maintain later on.

If you mean that there is no attribution for those icons specifically, I did add it. But if you mean that just showing that it is from font awesome is not enough then I will still lean towards hosting them ourselves, that way we have all the icons locally, and they can just be added by utilizing their respective classes in an (icon) element. This does add more size to the project, but it also reduces the styling necessary to make sure the svgs show properly and assures a more consistent style/size.

@egekorkan
Copy link
Member

comments are added as expected, merging

@egekorkan egekorkan merged commit 1071dcb into eclipse-thingweb:master Jan 21, 2024
10 of 12 checks passed
@SergioCasCeb SergioCasCeb deleted the changing-icons-to-svgs-sergio branch January 22, 2024 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Collapse/Open icons are reversed Changing icons to SVG
2 participants