Paths

Table of Contentst

Differential D3666

[web] Center modals vertically
ClosedPublic
Actions

Authored by • jacek on Apr 8 2022, 4:20 AM.

Tags

None

Referenced Files

	F3185270: D3666.diff
	Fri, Nov 8, 12:44 PM

	Unknown Object (File)
	Fri, Nov 8, 5:05 AM

	Unknown Object (File)
	Fri, Nov 8, 2:05 AM

	Unknown Object (File)
	Fri, Nov 8, 1:16 AM

	Unknown Object (File)
	Fri, Nov 8, 1:06 AM

	Unknown Object (File)
	Tue, Nov 5, 4:10 AM

	Unknown Object (File)
	Tue, Nov 5, 4:10 AM

	Unknown Object (File)
	Tue, Nov 5, 4:10 AM

Subscribers

Details

Reviewers

tomek
atul
• benschac

Commits

rCOMMff68fc69e922: [web] Center modals vertically

Summary

According to the designs, modals should be centered vertically. The change removes static top margin and centers the modal component

Test Plan

In web app, all existing modals should display in the center of view.

Diff Detail

Repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• jacek created this revision.Apr 8 2022, 4:20 AM

Herald added subscribers: • abosh, • benschac, • karol and 4 others. · View Herald TranscriptApr 8 2022, 4:20 AM

Harbormaster completed remote builds in B8018: Diff 11223.Apr 8 2022, 4:25 AM

• jacek requested review of this revision.Apr 8 2022, 4:25 AM

• jacek added a child revision: D3667: [web] Convert `Modal` into functional component.Apr 8 2022, 6:02 AM

• jacek added a parent revision: D3665: [web] Allow displaying multiple modals.

• jacek added reviewers: tomek, atul, • benschac.Apr 8 2022, 6:05 AM

atul accepted this revision.Apr 8 2022, 10:31 AM

This revision is now accepted and ready to land.Apr 8 2022, 10:31 AM

(would be helpful to attach a screenshot for diffs like this in the future)

• benschac added inline comments.Apr 8 2022, 10:32 AM

web/modals/modal.css
12 ↗	(On Diff #11223)	For centering modals I think transform translate might be a better fit: https://wattenberger.com/blog/css-percents https://stackoverflow.com/questions/6810031/css-center-element-within-a-div-element

• benschac requested changes to this revision.Apr 8 2022, 10:33 AM

This revision now requires changes to proceed.Apr 8 2022, 10:33 AM

ashoat added inline comments.Apr 10 2022, 7:15 PM

web/modals/modal.css
12 ↗	(On Diff #11223)	What's the impact on performance of each approach?

• jacek requested review of this revision.Apr 14 2022, 2:59 AM

• jacek added inline comments.

web/modals/modal.css
12 ↗	(On Diff #11223)	@benschac Why is `transform: translate(...)` better here? We would have to introduce `position absolute` and `top: 50%, left: 50%`, on modal-container, to use it, don't we? As we already use flex positioning inside `modal-overlay` and center horizontally using `align-items`, using `justify-content` to center vertically seems to be a natural solution for me, and it's simple. Doesn't transforms have worse performance that aligning with flex?

• benschac accepted this revision.Apr 14 2022, 2:42 PM

• benschac added inline comments.

web/modals/modal.css
12 ↗	(On Diff #11223)	I googled around a bit more. This CSS makes sense and is a bit more readable. I don't think there's added benefit to my previous suggestion. Sorry about that.

This revision is now accepted and ready to land.Apr 14 2022, 2:42 PM

rebase

Harbormaster completed remote builds in B8517: Diff 11876.Apr 25 2022, 10:33 AM

rebase

Harbormaster completed remote builds in B8552: Diff 11921.Apr 26 2022, 8:39 AM

Closed by commit rCOMMff68fc69e922: [web] Center modals vertically (authored by • jacek). · Explain WhyApr 26 2022, 9:09 AM

This revision was automatically updated to reflect the committed changes.

• jacek added a commit: rCOMMff68fc69e922: [web] Center modals vertically.

Revision Contents
Changeset List

Path

Size

web/

modals/

7 lines

9 lines

Diff 11932

web/modals/modal.css

Loading...

web/modals/modal.react.js

Loading...