Page MenuHomePhabricator

[web] [refactor] [ENG-716] modal design to match figma
ClosedPublic

Authored by benschac on Apr 5 2022, 12:30 PM.
Tags
None
Referenced Files
F3358616: D3624.diff
Sun, Nov 24, 5:39 AM
Unknown Object (File)
Fri, Nov 15, 9:03 AM
Unknown Object (File)
Wed, Nov 13, 5:12 PM
Unknown Object (File)
Wed, Nov 13, 5:11 PM
Unknown Object (File)
Wed, Nov 13, 5:11 PM
Unknown Object (File)
Wed, Nov 13, 5:11 PM
Unknown Object (File)
Wed, Nov 13, 5:11 PM
Unknown Object (File)
Wed, Nov 13, 5:11 PM

Details

Summary
Test Plan

click leave thread, leave modal thread opens, matches figma

Modal should be an exact match to the design. Letter wrapping and padding should match

figma:

Image 2022-04-06 at 10.22.53 AM.jpg (544×1 px, 50 KB)

proposed:
Image 2022-04-05 at 3.19.34 PM.jpg (524×1 px, 62 KB)

Diff Detail

Repository
rCOMM Comm
Branch
arcpatch-D3624
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 5 2022, 12:30 PM
Harbormaster failed remote builds in B7898: Diff 11075!
benschac retitled this revision from [web] [modal] [ENG-716] modal design to match figma to [web] [refactor] [ENG-716] modal design to match figma.
benschac edited the summary of this revision. (Show Details)
benschac added reviewers: tomek, jacek.
Harbormaster returned this revision to the author for changes because remote builds failed.Apr 5 2022, 12:35 PM
Harbormaster failed remote builds in B7899: Diff 11076!
tomek added inline comments.
web/modals/threads/confirm-leave-thread-modal.css
2 ↗(On Diff #11085)

You can use a shorthand

4–5 ↗(On Diff #11085)

Do we need to set these? I think they should be set by a modal

7 ↗(On Diff #11085)

Not related to this diff, but it would be significantly easier if we had this set for everything

Based on Incomplete List of Mistakes in the Design of CSS https://wiki.csswg.org/ideas/mistakes

Box-sizing should be border-box by default

web/modals/threads/confirm-leave-thread-modal.react.js
26 ↗(On Diff #11085)

Nit: I would expect only a single button in button container - we can rename the class to buttonsContainer

This revision is now accepted and ready to land.Apr 6 2022, 11:05 PM
web/modals/threads/confirm-leave-thread-modal.css
2 ↗(On Diff #11085)

I'm confused, what shorthand is being suggested here? @palys-swm

7 ↗(On Diff #11085)

Yeah, this has come up before... we should add a CSS reset to the web project, but it will be a lot of work to verify we have no regressions as a result. CSS reset work is tracked in ENG-826

web/modals/threads/confirm-leave-thread-modal.css
2 ↗(On Diff #11085)

Left and right are equal, so we can use padding: 0 40px 32px

7 ↗(On Diff #11085)

That's really unfortunate that we didn't introduce it at the beginning of the redesign

web/modals/threads/confirm-leave-thread-modal.css
4–5 ↗(On Diff #11085)

Image 2022-04-15 at 5.28.43 PM.jpg (484×1 px, 41 KB)

text is black w/o. still need it.

This revision was landed with ongoing or failed builds.Apr 15 2022, 2:34 PM
This revision was automatically updated to reflect the committed changes.