Page MenuHomePhabricator

[web] Copy user-settings-modal to password-change-modal
ClosedPublic

Authored by tomek on Mar 21 2022, 3:34 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 6, 3:32 PM
Unknown Object (File)
Tue, Nov 5, 1:18 AM
Unknown Object (File)
Wed, Oct 23, 7:44 AM
Unknown Object (File)
Wed, Oct 23, 7:11 AM
Unknown Object (File)
Tue, Oct 15, 11:13 AM
Unknown Object (File)
Oct 4 2024, 1:36 PM
Unknown Object (File)
Sep 23 2024, 8:04 PM
Unknown Object (File)
Sep 23 2024, 8:04 PM

Details

Summary

user-settings-modal has password change functionality, so it can be reused. In near future, we're going to delete user-settings-modal completely, so copying the logic is fine for now. In this diff the files are copied, imports are fixed and component name is updated - all the remaining code is not touched.

Test Plan

Flow

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek requested review of this revision.Mar 21 2022, 3:41 AM

Makes sense to just copy-paste the CSS we have and not continue to share it in a bunch of different modals.

This revision is now accepted and ready to land.Mar 21 2022, 6:53 AM

Makes sense to just copy-paste the CSS we have and not continue to share it in a bunch of different modals.

I don't think copy-pasting is a good long-term solution. I think instead, the shared CSS should be kept in one case, and used in some generic Modal component (or something like that) so that the CSS file is only accessed by one React component.

ashoat requested changes to this revision.Mar 21 2022, 8:20 AM

Requesting changes to get an idea of what our long-term plan is for sharing CSS like this without copy-pasting it

This revision now requires changes to proceed.Mar 21 2022, 8:20 AM
tomek requested review of this revision.Mar 21 2022, 8:35 AM
In D3472#94527, @ashoat wrote:

Requesting changes to get an idea of what our long-term plan is for sharing CSS like this without copy-pasting it

Totally agree that copying the css is not a good solution. In the following diffs (already in this stack) most of the css is deleted / replaced by the new implementation. This decision was made because I wanted to reuse as much logic as I can and at the same time make the review process easier. So to do that, there were two options: either modify the existing settings modal to be more reusable or copy it and modify. What makes the second option better is that the original modal is going to be deleted soon, so at that point there will be no duplication.

Regarding long-term solution to modal styles, or styles in general, I don't like referencing styles from a couple of different components. That makes maintaining them really hard, as we need to check every usage when changing anything. It's even more complicated, when instead of simple classes we're using more sophisticated selectors.
The most maintainable solution would be to have a set of components that provide layout or functionality and are used as e.g. containers - exactly how you suggested:

I think instead, the shared CSS should be kept in one case, and used in some generic Modal component (or something like that) so that the CSS file is only accessed by one React component.

This stack takes this approach by using Modal component, that should contain all the styles and logic necessary. In the future, we could extend this approach, by e.g. providing lists, cards, etc.

ashoat removed a reviewer: ashoat. ashoat added 1 blocking reviewer(s): atul.Mar 21 2022, 8:42 AM

Thanks, that explanation makes sense! Removing myself from review since I haven't had a chance to review everything, and adding @atul as a blocking reviewer

Looks good!

Note: I didn't read through the entire diff, here's what I did to review:

  1. Copy web/settings/password-change-modal.css (from this diff) into web/modals/account/user-settings-modal.css (on master) and ran git diff to make sure the files are identical.
  2. Copy web/settings/password-change-modal.js (from this diff) into web/modals/account/user-settings-modal.react.js (on master) and ran git diff to check the changes:

eb46.png (246×2 px, 375 KB)

7440.png (42×2 px, 76 KB)

758e.png (1×2 px, 1 MB)

and verified that "imports are fixed and component name is updated - all the remaining code is not touched."

This revision is now accepted and ready to land.Mar 22 2022, 11:55 AM