-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Address follow-up work for #14639 #14650
Merged
Merged
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
f922763
Add test cases for not-yet-migrateable configs
philipp-spiess c73242b
Improve comments and API naming
philipp-spiess 41617f9
Update test names
philipp-spiess 0630bb9
Tweak test name
thecrypticace 635105a
cleanup a bit
thecrypticace bdd966b
Don’t migrate themes with overly nested objects / values
thecrypticace c4ba9ee
tweak test
thecrypticace fb27131
Add test to verify theme.extend has one extra level of nesting
thecrypticace 0f68854
Don’t merge invalid config keys into the theme
thecrypticace afb3d86
Only add a `@theme` block when there are theme keys
philipp-spiess 9b46de8
Merge remote-tracking branch 'origin/next' into chore/address-static-…
philipp-spiess a2ff8cc
Clean up tests and document typography behavior that works now
philipp-spiess 7daed84
Only migrate the config file if all top-level theme keys are allowed
philipp-spiess 4a2a624
Add second PR to changelog
philipp-spiess File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we specifically check for typography or do we bail on any complex object under
theme
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bails out right now because it's using a function inside
theme
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it so should that be how the test is named then since it's not really about typography stuff? It's just testing another heuristic right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also do we have a heuristic that prevents deeply nested objects as theme values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we do, but this is what this should eventually be, I think. I think in the compat layer right now these typography values would be added to the
Theme
, I did not see any heuristic trying to remove it.We'll need to be able to support functions in the theme object and when we do we have to figure out how to still not add stuff for typography. That's really the two differences here.
Right now that test is not different from the one above "does not upgrade JS config files with functions in the theme config", so we can drop it. I just wanted to be explicit about this case since when we add support for the one above, we probably don't want to forget that this should not do weirdly nested stuff now.