Navigation: Force text decoration styles on nav item in editor #35859
Conversation
aaronrobertshaw
added
[Feature] Design Tools
|
Size Change: +1.12 kB (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
Thanks for putting this together @aaronrobertshaw!
From the comment around where the |
That's a much cleaner approach. ✨ I'll switch to that. It works well for me and I couldn't find any regressions in terms of being able to edit or style each item. Thanks!
If we were to revert to the original approach down the road, there was a decision made when the text-decoration block support was introduced, that it only support Thanks for your time reviewing this one! |
There was a problem hiding this comment.
Thanks for switching that around @aaronrobertshaw! This is testing well for me (tested in FF, Chrome, Safari, and Edge on desktop). Just added a comment about the comment we should probably update with going with this approach. But other than that, LGTM!
Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com>
| @@ -20,9 +20,9 @@ | |||
| } | |||
| } | |||
|
|
|||
| // This element is inline on the frontend but needs to be editable, therefore inline-block, in the editor. | |||
| // This element is inline on the frontend but needs to be editable, therefore ensure that the RichText field is inline in the editor. | |||
There was a problem hiding this comment.
If it's inline on the frontend, and it works as inline in the editor also (I could swear it needed to be inline-block for some reason), then this entire rule can be removed from the editor style, no?
|
I haven't had time to test this one deeply, and I don't mean to block this PR from landing. But there's a challenge described and discussed in #34275, which suggests to me that we might actually want to toggle off the text-decoration feature for navigation entirely, at least until it can be applied correctly. To summarize the issue: text-decoration has a fairly unique behavior in that it gets inherited all the way down the cascade, and cannot be unset! If an ancestor has As it is, this inheritance means that if you add a search block inside navigation, the input field of the search box will also inherit the underline 🙈 So the right fix to that is to apply the text-decoration property not on the navigation block itself, but target only menu items. (Perhaps a small fix is to apply a "has-text-decoration-underline" class instead of the inline style, and then scope it to menu items 🤔 ). Alternately, we should consider turning the feature off entirely. |
|
Thanks for the extra detail @jasmussen 👍 I'm actually in the process of reviewing #34064 which will opt-out of the text-decoration support for the navigation block. I'm happy to abandon this PR and wait for that to land instead. This was a small follow-up to address an issue uncovered during the testing for #33744. I don't believe there is any hurry to merge this as an interim solution. |
|
I don't think this PR needs to necessarily be abandoned. In fact if that inline block rule can be removed (I don't recall why it was necessary), then that alone feels like a nice little cleanup. |
|
Ok, sounds like a plan. I'll give the removal of the inline style a test Monday and can proceed from there. |
|
After some testing, it looks like simply removing the |
|
I appreciate the amount of red in this one, and since the PR only fixes a bug that ships in trunk, it seems fine to land this one. However I still think we need to consider how we are going to solve #34275. If we are confident that we can keep the text-decoration control where it is, and target all navigation items inside instead of the navigation block itself — then we can probably live with this for a bit. I.e. we want this: ... or this: But not this, which is currently shipping: If we are not confident in fixing the above as a bugfix for 5.9, we should probably disable the text-decoration control for the time being. What do you think? |
I believe this is fair. The linked issue is outside the scope of what this PR itself aims to address.
I'm not sure we'll get a consensus and fix in place within the period of time we have left before the 5.9 code freeze. The navigation block opted into the text-decoration support before we had the ability to skip serialization of those styles and apply them to inner blocks or elements. Now we have that, there might be the option to keep the control at the main navigation block level and pass that style down or apply a custom class such as I can explore this further tomorrow.
This is a little more involved than simply opting out of the text-decoration support. It has been enabled for about a year so we'd have to add a deprecation, then undo that in the future. Disabling the support only, would mean users losing text-decoration selections they'd made previously. We'd also need to test for possible block invalidation but my hope would be because the block support only applies an inline style we'd avoid that. There is also currently an open PR around which controls display by default under the Typography panel. For what it is worth, text-decoration will not be displayed by default. Given all of the above, are you still happy for us to land this and at least remove one small bug? |
|
From this, it definitely sounds like either a fix, or just an opt-out, is possible in the bug fixing window, so definitely seems good to land! Thanks. |
|
My mistake, turns out this rule was needed after all: #36106. |
andrewserong
added
the
[Type] Bug
Related:
Description
This PR removes the
inline-blockstyle applied to theRichTextfield for navigation items in the editor. This allows text decoration styles to be inherited from the main navigation block when applied via block supports.How has this been tested?
Manually.
To replicate the issue, follow the description of the issue outlined in #33744 (review)
To test this PR fixes that problem.
Screenshots
Before
Screen.Recording.2021-10-22.at.2.57.00.pm.mp4
After
Screen.Recording.2021-10-22.at.2.55.30.pm.mp4
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).