Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fix visual bugs on AccessSecretStorageDialog #8160

Merged
merged 17 commits into from
May 11, 2022
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion res/css/_common.scss
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ limitations under the License.
@import "./_font-sizes.scss";
@import "./_font-weights.scss";
@import "./_animations.scss";
@import "./_spacing.scss";
@import url("maplibre-gl/dist/maplibre-gl.css");

$hover-transition: 0.08s cubic-bezier(.46, .03, .52, .96); // quadratic
Expand Down Expand Up @@ -407,7 +408,8 @@ legend {
}

.mx_Dialog_buttons {
margin-top: 20px;
margin-top: $spacing-20;
margin-inline-start: auto;
text-align: right;

.mx_Dialog_buttons_additive {
Expand All @@ -416,6 +418,22 @@ legend {
}
}

.mx_Dialog_buttons_row {
display: flex;
flex-wrap: wrap;
justify-content: flex-end;
text-align: initial;
margin-inline-start: auto;

// default gap among elements
column-gap: $spacing-8; // See margin-right below inside the button style
row-gap: 5px; // See margin-bottom below inside the button style

button {
margin: 0 !important; // override the margin settings
}
}

/* XXX: Our button style are a mess: buttons that happen to appear in dialogs get special styles applied
* to them that no button anywhere else in the app gets by default. In practice, buttons in other places
* in the app look the same by being AccessibleButtons, or possibly by having explict button classes.
Expand Down
77 changes: 49 additions & 28 deletions res/css/views/dialogs/security/_AccessSecretStorageDialog.scss
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

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.

Copy link
Contributor Author

@luixxiul luixxiul Apr 1, 2022

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...

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;
}

Expand Down Expand Up @@ -84,7 +60,7 @@ limitations under the License.
}

.mx_AccessSecretStorageDialog_recoveryKeyEntry_entryControlSeparatorText {
margin: 16px;
margin: $spacing-16;
}

.mx_AccessSecretStorageDialog_recoveryKeyFeedback {
Expand All @@ -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
}
}

Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -455,13 +455,12 @@ export default class CreateKeyBackupDialog extends React.PureComponent<IProps, I
if (this.state.error) {
content = <div>
<p>{ _t("Unable to create key backup") }</p>
<div className="mx_Dialog_buttons">
<DialogButtons primaryButton={_t('Retry')}
onPrimaryButtonClick={this.createBackup}
hasCancel={true}
onCancel={this.onCancel}
/>
</div>
<DialogButtons
primaryButton={_t('Retry')}
onPrimaryButtonClick={this.createBackup}
hasCancel={true}
onCancel={this.onCancel}
/>
</div>;
} else {
switch (this.state.phase) {
Expand Down
24 changes: 13 additions & 11 deletions src/components/views/elements/DialogButtons.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -100,17 +100,19 @@ export default class DialogButtons extends React.Component<IProps> {
return (
<div className="mx_Dialog_buttons">
{ additive }
{ cancelButton }
{ this.props.children }
<button type={this.props.primaryIsSubmit ? 'submit' : 'button'}
data-test-id="dialog-primary-button"
className={primaryButtonClassName}
onClick={this.props.onPrimaryButtonClick}
autoFocus={this.props.focus}
disabled={this.props.disabled || this.props.primaryDisabled}
>
{ this.props.primaryButton }
</button>
<span className="mx_Dialog_buttons_row">
{ cancelButton }
{ this.props.children }
<button type={this.props.primaryIsSubmit ? 'submit' : 'button'}
data-test-id="dialog-primary-button"
className={primaryButtonClassName}
onClick={this.props.onPrimaryButtonClick}
autoFocus={this.props.focus}
disabled={this.props.disabled || this.props.primaryDisabled}
>
{ this.props.primaryButton }
</button>
</span>
</div>
);
}
Expand Down
Loading