chore(lumx-icons): override twitter icon #1028
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gcornut
force-pushed
the
feat/icon-override-twitter
branch
3 times, most recently
from
3cf4837 to
28bcf88
Compare
gcornut
force-pushed
the
feat/icon-override-twitter
branch
2 times, most recently
from
6675dc1 to
d247232
Compare
gcornut
requested review from
lumautomation,
matmkian and
a team
as code owners
gcornut
force-pushed
the
feat/icon-override-twitter
branch
2 times, most recently
from
856b243 to
0231619
Compare
|
|
||
|
|
||
| // Bundle TS types in D.TS files | ||
| const bundleType = { |
There was a problem hiding this comment.
Is there a specific reason why we need types on the released version for our icons?
There was a problem hiding this comment.
Yes the mdi icon types are really trivial but when you import non-typed module in TS project it can produce errors or warnings (depending on the project config). We did had types before from mdi (and from the v4-to-v5 alias file), I just replaced manual type definition with automatic .d.ts bundling (we already use on @lumx/react)
gcornut
force-pushed
the
feat/icon-override-twitter
branch
2 times, most recently
from
4a3c08d to
18a1cac
Compare
This reverts commit 6dcc77e1f63bc1a0590c3f38473550dffe7f4f1e.
gcornut
force-pushed
the
feat/icon-override-twitter
branch
from
18a1cac to
de8eacb
Compare
juanigalan91
approved these changes
Nov 23, 2023
|
Re-running an prerelease to re-test everything... |
|
Tested in another project using both JS & font icons
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
General summary
override/override-icons: contains SVG icons to override MDIoverride/alias-icons.js: config contains mapping from icon name to aliasesoverride/generate.js: script that generate the TS & Font/SCSS overrides and then TS & Font/SCSS aliasesWe'll use aliases to keep retro-compatibility when changing version or for brand renaming (ex: twitter). Overrides will help us update or restore brand logo because MDI is deprecating them.
Testing
alpha-icon-override-twitterhttps://github.com/lumapps/design-system/actions/runs/6956038315/job/18962918745mdiTwittershould still work but should have been swapped to the new X logomdiXTwittershould be the same asmdiTwitter.mdi-twittershould still work but should have been swapped to the new X logo.mdi-x-twittershould be the same as.mdi-twitter