[v4] [icons] fix: restore support for old IconName keys #4945
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
[v4] [icons] fix: restore support for old IconName keysPreviews: documentation | landing | table | modern colors demo |
styu
reviewed
Oct 1, 2021
|
|
||
| /** | ||
| * This is a hacky implementation of a camelCase to Snake_Case string literal | ||
| * type converter... it works up to 30 characters, which is fine for our |
There was a problem hiding this comment.
is there somewhere closer to where icons are defined that we should mention this 30 character limit?
There was a problem hiding this comment.
not really 😅 it's up to the icons reviewer (me) to enforce it.
to be fair though, it only affects the older naming convention... if you have a new icon added to the set and it's greater than 30 chars... nothing will really break, you just won't be able to reference it as IconNames.SOMETHING_VERY_LONG_NOT_SURE_WHY_IM_SCREAMING... you'll just need to simmer down and use pascal case
There was a problem hiding this comment.
ha ok. in fairness, prefer just using the string literal anyway
change-case is now a regular dependencyPreviews: documentation | landing | table | modern colors demo |
styu
approved these changes
Oct 1, 2021
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.
Changes proposed in this pull request:
Restore support for icon names access via
IconNames.ERROR, while retaining support for the new syntax (IconNames.Error).This breaking change happened as a result of the switch to
fantasticonas the icon build tool, and although IMO it looks cleaner with the pascal case syntax, the costs of this breaking change (thousands of lines of meaningless code diff) outweigh the benefits of the code looking ever so slightly cleaner.Originally I thought I would be able to accomplish this using a custom
fantasticontemplate (similar to how we havescssandcsstemplates), but it turns out thattstemplates are unsupported. So instead I had to resort to some hacky type mapping...Reviewers should focus on:
The type hacks are not very pretty, but they don't really get expose to consumers in their TS build, so I think it's ok...
Screenshot
N/A