Font Appearance Control: Fix error in global styles for Site Title in TT1-Blocks #34520
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
|
Size Change: +14 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
Comment on lines
147
to
152
| const currentSelection = | ||
| selectOptions.find( | ||
| ( option ) => | ||
| option.style.fontStyle === fontStyle && | ||
| option.style.fontWeight === fontWeight | ||
| ) || selectOptions[ 0 ]; |
There was a problem hiding this comment.
I don't think this fallback to selectOptions[ 0 ] is needed.
The new check within getDescribedBy prevents the error. You could also argue that the fact that no explicit selection has actually been made is obscured by this.
There was a problem hiding this comment.
Thanks for testing @aaronrobertshaw, I've reverted this line and moved the fix over to the CustomSelectControl component. From taking a look at the error from downshift, it looks like setting the selectedItem to an empty string should do the trick (here's where other folks have encountered the issue previously).
I've added in an optional chaining to another line that expected a currently selected item just in case, too.
There was a problem hiding this comment.
Thanks for the quick turnaround. In further testing of the old approach, I noticed that the initial change from no selection to a non-default option did not actually take effect.
After the latest updates to this PR, I'm seeing what appears to be a related issue but slightly different as now the default option displays as blank.
| Before | After |
|---|---|
 |
 |
There was a problem hiding this comment.
Good catch @aaronrobertshaw! Since it's near the end of our week, I've reverted the change to the selectedItem so that this PR becomes just a fix for the two areas where describedBy is expecting an object. I'll write up the downshift issue separately so I can keep digging into that next week.
There was a problem hiding this comment.
Added a follow-up issue linking back here: #34524
…o avoid downshift error when there is no selected item
aaronrobertshaw
approved these changes
Sep 3, 2021
There was a problem hiding this comment.
As discussed, with the scope reduced to only fixing the fatal error thrown via getDescribedBy I think this is good to go 👍
With the current changes, the font appearance control correctly displays the selected item or the default and updates it appropriately. A separate PR to address the downshift error sounds like a plan.
Thanks again for fixing this!
7 tasks
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.
Description
Update: while this PR is merged, and resolves the fatal that was being thrown, the
downshifterror thrown in the console hasn't been resolved. I've opened up a separate issue to track that, and will look into a fix next weekWhile testing global styles using the TT1 Blocks theme, which sets a font weight but not a font style in its
theme.jsonfile, I noticed that selecting the Site Title block within global styles currently throws a fatal in the editor (see screenshots below).This appears to be because the
getDescribedByfunction attempted to get the.nameproperty on thecurrentSelectionobject, which starts out being undefined when running with this theme.The fix I've gone for is to set the current selection to the first option (Default) if none are available rather than letting
currentSelectionbeing undefined. This also resolves a warning thrown bydownshiftif theCustomSelectControlreceives an undefined value for the current selection.There might be a better way of handling the current or default value when the theme provides a mix between undefined and a real value for the appearance (as in TT1-Blocks), but for the moment at least, this seems to resolve the fatal, and the Site Title block can be customised within global styles.
How has this been tested?
These are the steps I used to test:
|| selectOptions[0]in this PR and see thatdownshiftthrows an error (the below screenshot) when switching between Default and a style in the Appearance control.With this PR applied:
Screenshots
Fatal thrown when selecting the Site Title block within global styles:
Error from
downshiftif we just resolve the issue with the.nameingetDescribedBy:Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).