This repository has been archived by the owner on Sep 11, 2024. It is now read-only.
-
-
Notifications
You must be signed in to change notification settings - Fork 828
Fix visual bugs on AccessSecretStorageDialog #8160
Merged
kerryarchibald
merged 17 commits into
matrix-org:develop
from
luixxiul:AccessSecretStorageDialog
May 11, 2022
Merged
Changes from 11 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
b66acd1
Remove duplicate mx_Dialog_buttons
luixxiul 09e461a
Group buttons on mx_Dialog with span
luixxiul c9a09a8
Move common rules of mx_Dialog_buttons_row to _common.scss
luixxiul 5016666
Spacing variables
luixxiul 78bbdb6
Nesting - .mx_AccessSecretStorageDialog_reset
luixxiul 84d3957
Merge branch 'develop' into AccessSecretStorageDialog
luixxiul 778bb02
Merge branch 'develop' into AccessSecretStorageDialog
luixxiul 45ba21a
Merge branch 'develop' into AccessSecretStorageDialog
luixxiul 022f06b
Merge branch 'develop' into AccessSecretStorageDialog
luixxiul 4822363
Merge branch 'develop' into AccessSecretStorageDialog
luixxiul d861438
Merge branch 'develop' into AccessSecretStorageDialog
luixxiul 388ec55
Remove unnecessary rule
luixxiul ebe9f62
Merge branch 'develop' into AccessSecretStorageDialog
luixxiul ad13142
Merge branch 'develop' into AccessSecretStorageDialog
luixxiul f3b2ff9
Merge branch 'develop' into AccessSecretStorageDialog
luixxiul c317094
Merge branch 'develop' into AccessSecretStorageDialog
luixxiul 3cf798d
Merge branch 'develop' into AccessSecretStorageDialog
luixxiul File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,38 +14,14 @@ See the License for the specific language governing permissions and | |
limitations under the License. | ||
*/ | ||
|
||
.mx_AccessSecretStorageDialog_reset { | ||
position: relative; | ||
padding-left: 24px; // 16px icon + 8px padding | ||
margin-top: 7px; // vertical alignment to buttons | ||
margin-bottom: 7px; // space between the buttons and the text when float is activated | ||
text-align: left; | ||
|
||
&::before { | ||
content: ""; | ||
display: inline-block; | ||
position: absolute; | ||
height: 16px; | ||
width: 16px; | ||
left: 0; | ||
top: 2px; // alignment | ||
background-image: url("$(res)/img/element-icons/warning-badge.svg"); | ||
background-size: contain; | ||
} | ||
|
||
.mx_AccessSecretStorageDialog_reset_link { | ||
color: $alert; | ||
} | ||
} | ||
|
||
.mx_AccessSecretStorageDialog_titleWithIcon::before { | ||
content: ''; | ||
display: inline-block; | ||
width: 24px; | ||
height: 24px; | ||
margin-right: 8px; | ||
margin-inline-end: $spacing-8; | ||
position: relative; | ||
top: 5px; | ||
top: 5px; // TODO: spacing variable | ||
background-color: $primary-content; | ||
} | ||
|
||
|
@@ -84,7 +60,7 @@ limitations under the License. | |
} | ||
|
||
.mx_AccessSecretStorageDialog_recoveryKeyEntry_entryControlSeparatorText { | ||
margin: 16px; | ||
margin: $spacing-16; | ||
} | ||
|
||
.mx_AccessSecretStorageDialog_recoveryKeyFeedback { | ||
|
@@ -97,7 +73,7 @@ limitations under the License. | |
mask-repeat: no-repeat; | ||
mask-position: center; | ||
mask-size: 20px; | ||
margin-right: 5px; | ||
margin-inline-end: 5px; // TODO: spacing variable | ||
} | ||
} | ||
|
||
|
@@ -120,3 +96,48 @@ limitations under the License. | |
.mx_AccessSecretStorageDialog_recoveryKeyEntry_fileInput { | ||
display: none; | ||
} | ||
|
||
.mx_AccessSecretStorageDialog_primaryContainer { | ||
.mx_Dialog_buttons { | ||
$spacingStart: $spacing-24; // 16px icon + 8px padding | ||
|
||
text-align: initial; | ||
display: flex; | ||
flex-flow: column; | ||
gap: 14px; // TODO: spacing variable | ||
|
||
.mx_Dialog_buttons_additive { | ||
float: none; | ||
|
||
.mx_AccessSecretStorageDialog_reset { | ||
position: relative; | ||
padding-inline-start: $spacingStart; | ||
|
||
&::before { | ||
content: ""; | ||
display: inline-block; | ||
position: absolute; | ||
height: 16px; | ||
width: 16px; | ||
left: 0; | ||
top: 2px; // alignment | ||
background-image: url("$(res)/img/element-icons/warning-badge.svg"); | ||
background-size: contain; | ||
} | ||
|
||
.mx_AccessSecretStorageDialog_reset_link { | ||
color: $alert; | ||
} | ||
} | ||
} | ||
|
||
.mx_Dialog_buttons_row { | ||
gap: $spacing-16; // TODO: needs normalization | ||
padding-inline-start: $spacingStart; | ||
|
||
button { | ||
margin: 0 !important; // override rules on common.scss | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is the same style as common.scss, do you still need it? |
||
} | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change in nesting strictly necessary? To me, it looks like these selectors are sufficiently specific. Unnecessary nesting of the styles makes them harder to read and harder to maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Selector's semantic specificity itself does not ensure correct styling, as you have seen on #8089 too. Nesting should protect you from possible and unnoticed visual regressions otherwise, which cannot be easily detected without automated tests. Without a policy it is a matter of preference to decide how strictly nesting should be, but this case if strict nesting had been applied, the bug of the link color of
.mx_AccessSecretStorageDialog_reset_link
unnoticed for a long time should have not happened in the first place. Nesting is not always required per se, but it is an effective insurance against visual regressions like above. You could loose the strictness later if needed, without causing unnoticed regressions. If rules are defined strictly, the necessity of maintaining them due to a visual bug is reduced from the start. It also should effectively ensure the independence of the components by disallowing unintentional references. This kind of proactive insurance must be what other preprocessors such as LESS also adopt nesting for, but I am afraid of it that it is just an amateur view of an outsider...