Conform the style rules of GeneralUserSettingsTab.tsx to the style guide
#10595
Conversation
general-user-settings-tab.spec.ts and apply style rules to the tab strictlygeneral-user-settings-tab.spec.ts and conform the style rules to the style guide
It is possible, and the test indeed should be landed at first, with a Percy snapshot test which I have forgotten to add. I am going to create another PR which cherry-picks the commit of the test, adjusting class names. Changing CSS rules will be properly tested then. I'm wondering if this would look fine. |
|
It sounds perfect to me! |
|
The PR for the test was created here: #10658 |
general-user-settings-tab.spec.ts and conform the style rules to the style guideGeneralUserSettingsTab.tsx to the style guide
|
@richvdh The snapshot (
|
| ) : null; | ||
|
|
||
| return ( | ||
| <div className="mx_SettingsTab_section"> | ||
| <div className="mx_SettingsTab_section mx_GeneralUserSettingsTab_section--discovery"> |
There was a problem hiding this comment.
This should not change how .mx_SettingsTab_subheading and .mx_Spinner are displayed, as both of them are elements inside threepidSection, which this element consists.
|
It looks like something is wrong with the test. I'm checking. |
|
@richvdh Thank you for the review. It should be fine now. |
|
#10679 should be merged at first. Merging this or that PR will result in a conflict which needs to be fixed. |
| .mx_GeneralUserSettingsTab_discovery_existing { | ||
| display: flex; | ||
| align-items: center; | ||
| margin-bottom: 5px; | ||
| } |
There was a problem hiding this comment.
Above .mx_GeneralUserSettingsTab_section--discovery_existing is nested, and setting this mx_GeneralUserSettingsTab_section--discovery_existing to a different level can lead to a regression due to the difference of specificity.
This means that mx_GeneralUserSettingsTab on this line above should be removed, in order to remove the difference of specificity altogether.
I am going to push a commit to address this.
There was a problem hiding this comment.
Addressed with 4cb0e59.
There was a problem hiding this comment.
And I remember that comment was directed to nesting of mx_GeneralUserSettingsTab_section--discovery_existing_address and mx_GeneralUserSettingsTab_section--discovery_existing_promptText into mx_GeneralUserSettingsTab_section--discovery_existing, so the nesting by this PR was not related to that comment anyway, because this PR was going to nest those selectors' declarations to mx_GeneralUserSettingsTab.
|
This has changed quite a bit since the earlier reviews, so I think it needs a full re-review. I don't think I'll have time today. |
There was a problem hiding this comment.
@luixxiul I'm afraid I'm still struggling to follow this. It's probably fine, but I am conscious of the risk of introducing bugs if anything is incorrect. I appreciate your work in adding a test, but it cannot detect every problem so we still need to review changes like this carefully.
One thing that makes it hard to review is that it seems to be conflating two different types of change into the same PR:
- Renaming classes like
mx_GeneralUserSettingsTab_accountSectiontomx_GeneralUserSettingsTab_section--accountto conform to the style guide - Restructuring the CSS to collect all the settings for a given class (such as
mx_SetIdServer) into one place.
Would it be possible to split up this PR to separate the structural changes from the renames?
|
I understood. I'll recreate the PR from removing commits for another PR, so please forget the current status of the PR. |
For elements inside "mx_GeneralUserSettingsTab_accountSection" and "mx_GeneralUserSettingsTab_discovery"
|
I updated the PR to exclude anything not related to conforming the file to our naming policy. |
|
Thanks for the review!
You would not have to be worried about my side as I am just doing that on my free will for free, following our style guide, to make the style codebase understandable to anyone, as easily as possible, for the sake of nothing more than the Matrix ecosystem. In this sense, I am a little bit sad because I feel you basically told me that it was a waste of time to review such a meaningless change. |
…guide (#10595) * Nesting: `mx_GeneralUserSettingsTab_changePassword` * Nesting: `mx_Spinner` * Conform the style rules to the naming policy For elements inside "mx_GeneralUserSettingsTab_accountSection" and "mx_GeneralUserSettingsTab_discovery" * Conform `mx_GeneralUserSettingsTab_discovery_existing*` to the naming policy
This PR intends to conform the style rules of general user settings tab to our updated style guide.
type: task
Signed-off-by: Suguru Hirahara luixxiul@users.noreply.github.com
Checklist
This change is marked as an internal change (Task), so will not be included in the changelog.