[web] cleanup password change modal
ClosedPublic
Actions

Authored by ginsu on Dec 7 2023, 11:36 AM.

Details

Reviewers

atul
rohan
kamil

Commits

rCOMM9dd68c7c57e0: [web] cleanup password change modal

Summary

This diff cleans up the one off styles in the password change modal and replaces it with styles of the redesigned modal.

As part of cleanup for this modal there were some creative liberties I took which I will outline below:

The old password change modal would display their error message to the left of the main CTA button

Screenshot 2023-12-05 at 5.56.43 PM.png (1×3 px, 766 KB)

However based on the new redesigned modals: "If there is only one button for the modal, then the button will fill up the entire space of the section"
I moved the error message to be below the last input of the modal
I also added a placeholder spacing (to match the height of the error message) below the last input so that if an error message is shown then the height of the modal will not change

Linear task: https://linear.app/comm/issue/ENG-5943/extendmodify-the-modal-props-api-to-follow-new-modal-designs

Depends on D10230

Test Plan

Please see screenshot below

default case:

Screenshot 2023-12-07 at 2.38.10 PM.png (1×3 px, 795 KB)

error case:

Screenshot 2023-12-07 at 2.38.17 PM.png (1×3 px, 779 KB)

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ginsu created this revision.Dec 7 2023, 11:36 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptDec 7 2023, 11:36 AM

ginsu edited the summary of this revision. (Show Details)Dec 7 2023, 11:43 AM

ginsu edited the test plan for this revision. (Show Details)

ginsu added reviewers: atul, rohan, kamil.

ginsu added a parent revision: D10230: [web] cleanup community roles modal.

ginsu edited the summary of this revision. (Show Details)

ginsu attached a referenced file: F916772: Screenshot 2023-12-07 at 2.38.17 PM.png. (Show Details)

ginsu attached a referenced file: F916771: Screenshot 2023-12-07 at 2.38.10 PM.png. (Show Details)

ginsu attached a referenced file: F916773: Screenshot 2023-12-05 at 5.56.43 PM.png. (Show Details)

ginsu added a child revision: D10234: [web] introduce subheader prop to modal component.Dec 7 2023, 11:53 AM

Harbormaster completed remote builds in B24864: Diff 34388.Dec 7 2023, 12:45 PM

ginsu requested review of this revision.Dec 7 2023, 12:45 PM

Does it make sense to define the error placeholder in the Modal component directly as an optional prop if we're going to need it in multiple places? Just an idea to enforce consistency

web/settings/password-change-modal.js
74–79 ↗	(On Diff #34388)	I know you just copied this over but maybe it makes sense to move this out of being defined in the `Button` component to make it a little cleaner

This revision is now accepted and ready to land.Dec 7 2023, 1:24 PM

ginsu added inline comments.Dec 7 2023, 2:05 PM

web/settings/password-change-modal.js
74–79 ↗	(On Diff #34388)	sweet yea i can def do that

Does it make sense to define the error placeholder in the Modal component directly as an optional prop if we're going to need it in multiple places? Just an idea to enforce consistency

I'll think more on this as I continue to work through cleaning the modals. It's not a bad idea, but my initial impression was that this error was more an exception rather than a rule. Let me circle back after I get a few more modals done/have a more informed decision

rebase before landing

Does it make sense to define the error placeholder in the Modal component directly as an optional prop if we're going to need it in multiple places? Just an idea to enforce consistency

created this linear task to track this suggestion: https://linear.app/comm/issue/ENG-6812/error-placeholder-prop

This revision was landed with ongoing or failed builds.Feb 15 2024, 12:10 AM

Closed by commit rCOMM9dd68c7c57e0: [web] cleanup password change modal (authored by ginsu). · Explain Why

This revision was automatically updated to reflect the committed changes.

ginsu added a commit: rCOMM9dd68c7c57e0: [web] cleanup password change modal.

Harbormaster completed remote builds in B26852: Diff 37116.Feb 15 2024, 12:13 AM