Page MenuHomePhabricator

[web] [feat] [ENG-972] update modal header to match design
ClosedPublic

Authored by benschac on Apr 5 2022, 8:00 AM.
Tags
None
Referenced Files
F3359279: D3618.id11530.diff
Sun, Nov 24, 8:23 AM
Unknown Object (File)
Sun, Nov 3, 3:40 PM
Unknown Object (File)
Sun, Nov 3, 3:40 PM
Unknown Object (File)
Sun, Nov 3, 3:40 PM
Unknown Object (File)
Sun, Nov 3, 3:40 PM
Unknown Object (File)
Sun, Nov 3, 3:40 PM
Unknown Object (File)
Sun, Nov 3, 3:40 PM
Unknown Object (File)
Sun, Nov 3, 3:34 PM

Details

Summary
Test Plan

open modals, make sure header matches figma linked in linear task

Diff Detail

Repository
rCOMM Comm
Branch
leave-channel-thread
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

web/modals/modal.css
62

This can be shortened using the padding shorthand property. Because both the padding-left and padding-right are 40px, the last 40px is extraneous and can be removed.

Read more: W3Schools

web/modals/modal.css
62

ty! should have done that initially!

tomek requested changes to this revision.Apr 6 2022, 7:35 AM
tomek added inline comments.
web/modals/modal.css
71–74 ↗(On Diff #11077)

I think we should try to avoid styles like that. This style means that we're going to apply it to every span no matter how deep into the structure. Also, we're hardcoding the fact that we use span which makes refactoring harder.

The main issue with this approach is about maintainability. There's no way to know in jsx that we apply some style to that span. It makes it really easy to make a change which breaks the styling. If we want to avoid the mistake and search for all the styles that are applied to the span, it would be almost impossible.

A lot more maintainable approach would be to use a class instead.

This revision now requires changes to proceed.Apr 6 2022, 7:35 AM
benschac added inline comments.
web/modals/modal.css
71–74 ↗(On Diff #11077)

I'm not sure what you mean.

https://developer.mozilla.org/en-US/docs/Web/CSS/Child_combinator -- the selector is:

just the h2 directly inside of the modal header, and then the span inside of that. I don't see a world where there are multiple nested spans inside this h2.

tomek added inline comments.
web/modals/modal.css
71–74 ↗(On Diff #11077)

I'm not sure what you mean.

Using descendant selectors is bad for maintainability because it's hard to discover all the styles that will be applied to an element.

I don't see a world where there are multiple nested spans inside this h2.

The fact that you don't see it doesn't mean that in the future someone wouldn't decide to do that. Also, it is time consuming to change span to something else because you would need to check a lot of css.

Not going to block you on this, but please keep maintainability in mind while designing solutions.

This revision is now accepted and ready to land.Apr 7 2022, 11:33 PM

It looks like this diff "pushes" the Modal title to the right. This probably makes sense when there is an icon, but it looks strange when there isn't.

We should either:

A. Decide that we do want to display an icon for every modal, in which case we should go through them and decide which ones
B. Justify the title to the left or center it if there's no icon

Otherwise modals (eg ThreadSettingsModal) end up looking pretty strange.

Feedback to @benschac about regressions

Disappointing to see a regression after I took the time to type this out on the corresponding Linear task two weeks ago:

Screen Shot 2022-04-19 at 12.05.14 AM.png (180×1 px, 51 KB)

@benschac, regarding the continued shipping of regressions... would be great if you could try integrating some of the feedback I've given to you about this previously, such as to make sure you have a theoretical understanding of the code changes you are making, not just a practical one. Your test plan shouldn't just be "test to see if it looks good" – it should be fully and thoroughly understanding every single code point this effects, and the side effects your change can have. That involves a LOT of git grep. One way to think about this: if you're not git greping somewhere in the course of writing a diff like this, then you probably aren't understanding your changes well enough.

Feedback to @atul about leaving comments requiring follow-ups without creating tasks

See this comment. Leaving a comment on a landed diff like this without a follow-up task is extra bad. Even if the diff is live, you should never be discussing a follow-up without linking a task for that follow-up. Please consider this to be a hard rule.

Feedback to @palys-swm about being stricter on test plan

In the review here, we should've been more strict with @benschac about testing every single codepoint his change effects. Especially as this is an issue that has come up, please make sure you bring this up repeatedly in every review. Every single codepoint should be checked and tested, and we should be very sensitive to potential regressions.

Feedback to @palys-swm about being stricter on test plan

In the review here, we should've been more strict with @benschac about testing every single codepoint his change effects. Especially as this is an issue that has come up, please make sure you bring this up repeatedly in every review. Every single codepoint should be checked and tested, and we should be very sensitive to potential regressions.

Thanks for this feedback - it makes sense! Going to do that during following reviews.