Fix-up Post Editor’s preferences modal #35369
Conversation
stokesman
added
[Type] Code Quality
|
Size Change: -6.66 kB (-1%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
stokesman
force-pushed
the
update/edit-post-preferences-navigator
branch
from
a03e720 to
5def9ad
Compare
stokesman
changed the title
| <Card isBorderless> | ||
| <CardBody> | ||
| <NavigatorProvider initialPath="/"> | ||
| <NavigatorScreen path="/"> | ||
| <NavigatorProvider initialPath="/"> | ||
| <NavigatorScreen path="/"> | ||
| <Card isBorderless size="small"> | ||
| <CardBody> |
There was a problem hiding this comment.
Putting the Cards and CardBodys inside the NavigatorScreens prevents the clipping of focus indicators and allows the root screen to use a padding different from the child screens.
stokesman
force-pushed
the
update/edit-post-preferences-navigator
branch
from
8ee8811 to
204fb37
Compare
There was a problem hiding this comment.
Hey @stokesman , thank you again for noticing the regression first and working on this PR!
I personally like the direction taken here — especially about removing custom CSS in favour of components composition.
I will also add other folks who know the preferences modal design and functionality a bit better to help with the review. (@jasmussen @ntsekouras and @jorgefilipecosta )
|
This is beautiful work. Notably I love how much bespoke CSS just for this one component has been removed, in favor of using the components to lay things out. Any layout issues present can then be fixed in the components themselves, ideally benefitting the whole system. This looks good at the wide breakpoint
It also looks pretty good at the small breakpoint, which I understand was the focus of this one:
However this one feels like a small step backwards: It's nice that the focus style is not clipped, but the button is fairly huge for what it needs to be. Can it be inline-block? The subheading, "Blocks", also feels like it sits alone, and with its normal font size and weight it looks like text rather than a heading. Can it be a big heading like it was before? An option would also be to have an icon-only back button, and then have the "Blocks" subsection item sit immediately after it, horizontally. |
stokesman
force-pushed
the
update/edit-post-preferences-navigator
branch
from
204fb37 to
cf7452b
Compare
|
Thanks for the review @jasmussen! I've pushed some updates—the main one to try your suggestion:
I didn't want to go back to the heading size as it was close to but slightly off from the modal header font size. I've made it the same size as the modal header, but kept it a lighter weight. Though it's not visually apparent, I also got rid of the actual I didn't update the snapshot this time around in case we need to make more changes. That's a test that will fail. |
| </CardBody> | ||
| </Card> | ||
| /> | ||
| <Text size="16" color="#1d2327"> |
There was a problem hiding this comment.
This color appears to come from some React Native files that we should not be using in most cases, and frankly should consider deprecating in the native pieces.
There was a problem hiding this comment.
Since this originally was a h2, ideally we should use Heading, but I'm afraid that it wouldn't support the styles required here (one more for #35464)
As an alternative, we could for now simply add as="h2" to the Text component
Also, I wonder if there's a better alternative to adding a hardcoded hex color inline? Maybe we should create a classname instead and add that styling to styles.scss (especially if that color already exists as a scss variable)
There was a problem hiding this comment.
The color shall be gone. It was only meant to be temporary until further updates bring color consistency. We can let the inconsistency be a spur for such improvements (if anyone is sensitive enough 😄 ).
Since this originally was a h2
An h2 could make sense here (in my first commit I had the <Text as="h2"…) but I realized all the headings that follow are also h2. If we simply make those h3 it's not correct at greater than medium breakpoints where the screen heading doesn't exist. Instead of adding any complexity to change heading level by breakpoint I decided to not use the h2. It seems better though it's definitely not the focus of the PR.
There was a problem hiding this comment.
This is what I see:
This is a step forward in a few ways:
- It replaces a ton of bespoke CSS in favor of using the new components. This is a massive gain, well done.
- The icon-only back works well
- It's a good small-viewport experience
Because of the code quality improvements, it does not preclude future and further enhancements.
It also does surface some challenges, though:
- There's still a
font-size: 0.9rem;rule in the code, which feels arbitrary and ill-advised. It also makes the back label 16px font size, which feels too big. But since this is existing code that you have not touched, the issue should not block this PR.
In that vein, it feels like enough of a step forward that following up on the font size is something that we could look at separately as we continue the extraction of bespoke CSS, which really is a noble goal.
I did leave one small note about a color that we shouldn't be using, and which can be removed before this PR lands.
Thank you.
There was a problem hiding this comment.
There's still a font-size: 0.9rem; rule in the code, which feels arbitrary and ill-advised. It also makes the back label 16px font size, which feels too big. But since this is existing code that you have not touched, the issue should not block this PR.
Yeah, definitely. Once components like Text and Heading feel a bit more mature, it'd be great also to re-write parts of the UI with it — and the preferences modal feels like a good self-contained piece of UI.
| </CardBody> | ||
| </Card> | ||
| /> | ||
| <Text size="16" color="#1d2327"> |
There was a problem hiding this comment.
Since this originally was a h2, ideally we should use Heading, but I'm afraid that it wouldn't support the styles required here (one more for #35464)
As an alternative, we could for now simply add as="h2" to the Text component
Also, I wonder if there's a better alternative to adding a hardcoded hex color inline? Maybe we should create a classname instead and add that styling to styles.scss (especially if that color already exists as a scss variable)
|
Thanks for reviewing again @jasmussen and @ciampo 🙇
Credit really goes to Riad for putting in the new component (while leaving the CSS 😄 ).
And there is indeed more of that to be done for the preferences modal. I am eager to land the fixes in this PR without getting into more of that for now. I might have exercised more restraint in these changes just to get the sure-wins in sooner. Thanks for taking it in stride Joen and Marco! |
Follow-up to #35142. This is meant to do a few things:
Navigator#35332).It's worth noting the approach for the second point on improving visuals is by edits to only component structure and props as opposed to adding custom CSS. The idea is to follow the approach Riad proposed:
#35142 (comment)
While the visuals can be further improved, some of it may be better done with careful improvement to component styles.
How has this been tested?
Manually.
Screenshots
Before (since #35332) clipping and sticky breakage are evident
edit-post-prefs-before-with-navigator-overflow.mp4
With this PR clipping and sticky issues resolved
edit-post-prefs-after-with-navigator-overflow.mp4
Blocks screen
BeforeAfter…Root menu
BeforeAfter…Types of changes
Enhancement
Checklist:
*.native.jsfiles for terms that need renaming or removal).