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.
Details
Flow
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Makes sense to just copy-paste the CSS we have and not continue to share it in a bunch of different modals.
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.
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.
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:
- 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.
- 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:
and verified that "imports are fixed and component name is updated - all the remaining code is not touched."