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

feat(styling)!: convert SVG icons to pure CSS #1474

Merged
merged 26 commits into from
Apr 26, 2024
Merged

feat(styling)!: convert SVG icons to pure CSS #1474

merged 26 commits into from
Apr 26, 2024

Conversation

ghiscoding
Copy link
Owner

@ghiscoding ghiscoding commented Apr 19, 2024

  • change how to colorize SVG icons to pure CSS approach by following AntFu icons in pure CSS (creator of UnoCSS), this will help colorize icons for a much better Light/Dark Mode support

TODOs

  • all unit tests should pass
  • all E2E should pass
  • remove any reference to Font Awesome "fa" icons
  • update the Styling SVG Icons doc about new ways to colorize SVG
    • also all docs referencing .mdi requires an update
  • pagination SVG icons & change disable button icon to gray
  • row selection checkboxes
  • ColumnPicker/GridMenu lists
  • Draggable Grouping is not correct in Dark Mode
  • Grouping/DraggableGrouping/TreeData (collapse/expand) icons
  • Sort icons
  • RowDetail icons
  • Autocomplete loading icon
    • this is an exception, we cannot make it pure CSS because we use ::after pseudo to display loading spinner within the input and it's only possible via content and we'll keep a light gray because we can only set the color once
  • remove all unused ms-select SASS variables
  • remove all references to .mdi classes and replace them by .sgi
    • decided to rollback and keep .mdi to make the version migration easier and smoother
  • remove all recolor SASS code and also color using CSS filter
  • we currently have $color- and $text-color- but with new pure CSS, we can probably merge into one.
    • keep only text-color-xx and delete previous SASS color-xx since all SVG can now use same color as any text
    • possibly make some dark mode alternative as well (e.g. custom text-color-primary and text-color-secondary in style.scss)
  • remove SASS variable $slick-icon-font-family since we no longer use any Fonts internally
  • remove old jQueryUI .ui-state and others

Build Size Comparison

I wasn't expecting much of a difference but I'm pleasantly surprised to see this minimal but nice drop in build size, that is with an improved use of pure CSS and a much better UI/UX :)

File before after diff % diff
SF Bundle Zip File 582Kb (596,961) 575Kb (589,262) 7Kb 1.29% smaller

- by using regular CSS, we can use regular font-size and even regular CSS color which was not possible with previous implementation, the technique is credited to UnoCSS project https://antfu.me/posts/icons-in-pure-css
- this is just a draft since it needs more investigation and it might actually turn out to be a breaking change because at the moment we use CSS `content` with `:before` pseudo to previously add SVG icons but with the new approach this might not be needed and might also mean that the default Font-Awesome (font) may no longer work, need investigation. - It might be a breaking change too for the users who configured different icons via content SASS vars. Unless we keep them for users but we defined them as unset.
Copy link

stackblitz bot commented Apr 19, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@ghiscoding ghiscoding marked this pull request as draft April 19, 2024 23:49
Copy link

codecov bot commented Apr 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.8%. Comparing base (0462f17) to head (1473903).
Report is 8 commits behind head on next.

Additional details and impacted files
@@           Coverage Diff           @@
##            next   #1474     +/-   ##
=======================================
+ Coverage   99.8%   99.8%   +0.1%     
=======================================
  Files        199     199             
  Lines      21714   21763     +49     
  Branches    7117    6994    -123     
=======================================
+ Hits       21650   21699     +49     
- Misses        58      64      +6     
+ Partials       6       0      -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

- found issues in pagination with new SVG icons and saw potential improvements to use BindingEventService instead of using too many querySelector, we can point directly to the HTML element and bind an event without querying anything
- also fix Sort icon hint that was broken (regression) since jQueryUI was removed
- since we use a `::after` pseudo, we cannot convert that SVG to pure CSS and so because the SVG is created once we cannot use the `currentColor` color, so we'll keep using a light gray color so that it works for both light/dark themes. Also note that is the last SVG left to address for migrating to pure CSS
- let's use our own SlickGridIcon (`.sgi`) everywhere instead
- increase default base sgi icon font size to 18px
- improve colors used in Dark Mode
@ghiscoding
Copy link
Owner Author

ghiscoding commented Apr 25, 2024

@zewa666 out of curiosity, which slickgrid theme and icons are you using (the default Slickgrid-Bootstrap maybe)? Because I'm doing a major revamp, as seen in TODO list above, and I'm renaming all .mdi CSS classes to .sgi because this fit better with the project itself (SlickGrid Icons) and mostly because the way to create the SVG is entirely different (now I'm using AntFu's approach for pure CSS as opposed to prior code using ::before pseudo with content). The native approach allows the user to colorize any SVG as opposed to before it was a fixed color... anyway long story short, I'd like to know what users use and if the rename is a huge thing (I guess it's ok to just do search and replace in the project) and also I might get rid of some SASS files that might be barely used (I created bare and lite versions for Material & Salesforce Themes but I'd be surprised if anyone uses them, the description is shown in comments below). I'd be happy to receive feedback before I merge this large PR which I'm nearly done with finally :)

"sass-build-task:scss-compile:material-bare": "sass src/styles/slickgrid-theme-material.bare.scss dist/styles/css/slickgrid-theme-material.bare.css --style=compressed --quiet-deps --no-source-map --load-path=node_modules",
"sass-build-task:scss-compile:material-lite": "sass src/styles/slickgrid-theme-material.lite.scss dist/styles/css/slickgrid-theme-material.lite.css --style=compressed --quiet-deps --no-source-map --load-path=node_modules",
"sass-build-task:scss-compile:salesforce": "sass src/styles/slickgrid-theme-salesforce.scss dist/styles/css/slickgrid-theme-salesforce.css --style=compressed --quiet-deps --no-source-map --load-path=node_modules",
"sass-build-task:scss-compile:salesforce-bare": "sass src/styles/slickgrid-theme-salesforce.bare.scss dist/styles/css/slickgrid-theme-salesforce.bare.css --style=compressed --quiet-deps --no-source-map --load-path=node_modules",
"sass-build-task:scss-compile:salesforce-lite": "sass src/styles/slickgrid-theme-salesforce.lite.scss dist/styles/css/slickgrid-theme-salesforce.lite.css --style=compressed --quiet-deps --no-source-map --load-path=node_modules",

// This is a bare version of "slickgrid-theme-material.scss",
// Few files were removed and aren't included in this styling theme (while they are in original theme)
// - (colors, extra-styling, material-svg-icons, material-svg-utilities, slick-without-bootstrap-min-styling)

/**
* SlickGrid Material Theme
* (sames as `slickgrid-theme-material.scss` but without Flatpickr & Multiple-Select styling)
* We also removed `slick-without-bootstrap-min-styling.scss` since that is causing issues with Bootstrap 4
*/

- since SVG can now be colorized the same way as regular text, we can merge previous recolor SASS utils and only keep native CSS text `color` definitions
- when user adds a `className` to SlickCustomTooltip, it was overriding the default class name and by doing so, it was breaking the CSS styling because the CSS Theme still expect the default CSS class name, so we need to append to the default instead of overriding it
- also fix a few more Dark Mode styling and provide alternate colors for Tooltip demo in Dark Mode
- also add alternate colors to `text-color-primary` and `text-color-secondary` for Dark Mode
@ghiscoding ghiscoding changed the base branch from master to next April 26, 2024 18:33
@zewa666
Copy link
Contributor

zewa666 commented Apr 26, 2024

I'm using both the full material and also bootstrap theme. if its not too much of a hassle I'd personally advocate for keeping them around as is. I'd really like to also push the grid for one of our SFDC use cases, so having that specific theme available would be a plus

@ghiscoding
Copy link
Owner Author

ghiscoding commented Apr 26, 2024

I'm using both the full material and also bootstrap theme. if its not too much of a hassle I'd personally advocate for keeping them around as is. I'd really like to also push the grid for one of our SFDC use cases, so having that specific theme available would be a plus

hmm maybe a misunderstanding, I am keeping the Material & Salesforce, however I also have extra bare and lite (e.g. slickgrid-theme-material.bare.scss) which don't include the SVG icons, I was thinking of possibly dropping those (not the default Material/Salesforce but really their lighter bundles)

I am undecided on the .mdi to .sgi change, I do prefer .sgi to represent SlickGrid Icons (which I have actually done in SlickGrid core lib) but I did think about potential users that might be holding their migration because of the size of search & replace. Also, the new icons are not aligned the same way either, in previous version I had to add some extra align bottom hacks because I was using ::before { content: 'svg...' } but now that it's pure CSS, it follows the same alignment and colors as any other text. Another potential conflict could be for anyone using the real Material Design Icon font package, then it could conflict if they already use .mdi for that font package. I know, I should have chosen .sgi from the start but...

I have already changed everything to .sgi in this PR, but the main reason was mostly to make sure I wasn't referencing the old SVGs and that I tested everything (UI), but now that I'm done then I guess I could go back to .mdi. I still have a preference for .sgi but I do understand users might be holding their migration because of the massive search & replace.

@zewa666 So you're not using any of the bare or lite, right? I'm thinking of dropping one or both of them. The idea was to bring lighter theme bundle without the .mdi SVG icons, but I'm assuming most user will want to use these icons anyway making these bundles pointless

Basic Migration Guide v5 is also included in this PR

- the idea was to renamed mdi to `.sgi` for SlickGrid Icons but the change would bring a ton of change on the user side, so let's keep using `.mdi` and table this aside for maybe another future major version (or not)
@ghiscoding
Copy link
Owner Author

ghiscoding commented Apr 26, 2024

I decided to rollback the .mdi to .sgi change to make the migration smoother and easier for the developers and avoid having users postponing their upgrades because of it. @zewa666 I'm still curious if you could reply to my previous paragraph though, thanks

I'm going to go ahead and merge this into the next branch 🎉

@ghiscoding ghiscoding marked this pull request as ready for review April 26, 2024 22:23
@ghiscoding ghiscoding merged commit 70cda8a into next Apr 26, 2024
7 checks passed
@ghiscoding ghiscoding deleted the feat/css-icons branch April 26, 2024 22:27
@zewa666
Copy link
Contributor

zewa666 commented Apr 27, 2024

I'll have to check on Tuesday when back in office about the lite/bare use

@zewa666
Copy link
Contributor

zewa666 commented May 2, 2024

we're using the full material theme:

@import "@slickgrid-universal/common/dist/styles/sass/slickgrid-theme-material.scss"

@ghiscoding
Copy link
Owner Author

good to know, I wasn't even sure if anyone were using the Material Theme. cool

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.

3 participants