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

Add default properties to link buttons and remove redundant settings #8151

Merged
merged 10 commits into from
Apr 5, 2022
Merged

Add default properties to link buttons and remove redundant settings #8151

merged 10 commits into from
Apr 5, 2022

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented Mar 25, 2022 •
edited by github-actions bot
Loading

Like a previous PR on link buttons, this PR sets additional properties to them as default ones. There should be no major regressions as I checked manually and set properties to elements which need specific settings.

This PR also sets display: inline to link_inline to reflect its name and cancel inline-flex inherited from mx_AccessibleButton_hasKind. inline-flex places button texts weirdly and breaks the design when they are over multiple lines.

Signed-off-by: Suguru Hirahara [email protected]

Notes: none

type: task

This change is marked as an internal change (Task), so will not be included in the changelog.

Preview: https://pr8151--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

luixxiul added 5 commits March 25, 2022 10:47
@luixxiul
Add font-size: inherit to link button
2bdecc9 
Remove redundant font-size settings

_FeedbackDialog.scss
_GenericFeatureFeedbackDialog.scss
_Login.scss
_NewRoomIntro.scss
_NotificationSettingsTab.scss
_PinnedEventTile.scss
_PreferencesUserSettingsTab.scss
_SpaceCreateMenu.scss
_ToastContainer.scss
_UserMenu.scss

Specify font-size
- _ProfileSettings.scss
- _SpaceBasicSettings.scss
- _SpaceSettingsDialog.scss

Signed-off-by: Suguru Hirahara <[email protected]>
@luixxiul
Add line-height: inherit to link button
45a6510 
Remove redundant setting
- _GenericFeatureFeedbackDialog.scss
- _PinnedEventTile.scss
- _SpaceCreateMenu.scss

Signed-off-by: Suguru Hirahara <[email protected]>
@luixxiul
Add font-weight: normal to link button
aee226f 
Remove redundant setting
- _SpotlightDialog.scss
- _UserMenu.scss

Signed-off-by: Suguru Hirahara <[email protected]>
@luixxiul
Dedupe _link and _link_inline
1742f01 
Signed-off-by: Suguru Hirahara <[email protected]>
@luixxiul
Set inline to "link_inline"
3352084 
Treat the button as its name indicates.
For elements that should not be inlined, "link" should be used.

Signed-off-by: Suguru Hirahara <[email protected]>
@luixxiul luixxiul requested a review from a team as a code owner March 25, 2022 11:06
@codecov
Copy link

codecov bot commented Mar 25, 2022 •
edited
Loading

Codecov Report

Merging #8151 (e288668) into develop (1f64835) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #8151   +/-   ##
========================================
  Coverage    28.68%   28.68%           
========================================
  Files          851      851           
  Lines        49778    49778           
  Branches     12657    12657           
========================================
  Hits         14281    14281           
  Misses       35497    35497

luixxiul
luixxiul commented Mar 25, 2022
View reviewed changes
res/css/structures/auth/_Login.scss
@@ -88,13 +88,12 @@ limitations under the License.
div.mx_AccessibleButton_kind_link.mx_Login_forgot {
display: block;
margin: 0 auto;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These will be able to be removed by creating a wrapper and setting kind="link_inline" in favor of link.

luixxiul
luixxiul commented Mar 25, 2022
View reviewed changes
res/css/views/dialogs/_SpaceSettingsDialog.scss
@@ -77,6 +77,7 @@ limitations under the License.

.mx_AccessibleButton_hasKind {
&.mx_AccessibleButton_kind_link {
font-size: $font-14px;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after2

luixxiul
luixxiul commented Mar 25, 2022
View reviewed changes
res/css/views/spaces/_SpaceBasicSettings.scss
@@ -65,6 +65,7 @@ limitations under the License.
display: inline-block;
margin: auto 18px;
color: #368bd6;
font-size: $font-14px; // See _SpaceSettingsDialog.scss
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after5

luixxiul
luixxiul commented Mar 25, 2022
View reviewed changes
res/css/views/spaces/_SpaceCreateMenu.scss
@@ -118,7 +114,5 @@ $spacePanelWidth: 68px;
.mx_AccessibleButton_kind_link {
color: $accent;
position: relative;
font-size: inherit;
line-height: inherit;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after

luixxiul
luixxiul commented Mar 25, 2022
View reviewed changes
res/css/views/rooms/_PinnedEventTile.scss
@@ -92,8 +92,6 @@ limitations under the License.

.mx_AccessibleButton_kind_link {
margin-left: 12px;
font-size: inherit;
line-height: inherit;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pinned

luixxiul
luixxiul commented Mar 25, 2022
View reviewed changes
res/css/views/rooms/_NewRoomIntro.scss
.mx_AccessibleButton_kind_link {
font-size: inherit;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newroom

luixxiul
luixxiul commented Mar 25, 2022
View reviewed changes
res/css/structures/_UserMenu.scss
.mx_AccessibleButton_kind_link {
font-weight: normal;
font-size: inherit;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guest

luixxiul
luixxiul commented Mar 25, 2022
View reviewed changes
res/css/views/dialogs/_FeedbackDialog.scss
.mx_AccessibleButton_kind_link {
font-size: inherit;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feedback

luixxiul
luixxiul commented Mar 25, 2022
View reviewed changes
res/css/views/dialogs/_GenericFeatureFeedbackDialog.scss
.mx_AccessibleButton_kind_link {
font-size: inherit;
line-height: inherit;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generic

luixxiul
luixxiul commented Mar 25, 2022
View reviewed changes
res/css/views/dialogs/_SpotlightDialog.scss
@@ -241,7 +241,6 @@ limitations under the License.
.mx_SpotlightDialog_recentSearches > h4 > .mx_AccessibleButton_kind_link {
padding: 0;
float: right;
font-weight: normal;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spot

luixxiul
luixxiul commented Mar 25, 2022
View reviewed changes
res/css/views/settings/tabs/room/_NotificationSettingsTab.scss

.mx_AccessibleButton_kind_link {
font-size: inherit;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

notification

luixxiul
luixxiul commented Mar 25, 2022
View reviewed changes
res/css/views/settings/_ProfileSettings.scss
@@ -61,6 +61,7 @@ limitations under the License.
margin-bottom: 28px;

> .mx_AccessibleButton_kind_link {
font-size: $font-14px;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

profile

luixxiul
luixxiul commented Mar 25, 2022
View reviewed changes
res/css/views/settings/tabs/user/_PreferencesUserSettingsTab.scss

.mx_AccessibleButton_kind_link {
font-size: inherit;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

preference

luixxiul
luixxiul commented Mar 25, 2022
View reviewed changes
res/css/structures/auth/_Login.scss
@@ -88,13 +88,12 @@ limitations under the License.
div.mx_AccessibleButton_kind_link.mx_Login_forgot {
display: block;
margin: 0 auto;
// style it as a link
font-size: inherit;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

login

@luixxiul luixxiul changed the title Set default properties to link button and remove redundant settings Set default properties to link buttons and remove redundant settings Mar 26, 2022
@github-actions github-actions bot added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Mar 26, 2022
@luixxiul luixxiul changed the title Set default properties to link buttons and remove redundant settings Add default properties to link buttons and remove redundant settings Mar 28, 2022
turt2live
turt2live approved these changes Apr 4, 2022
View reviewed changes
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is generally fine, though as a precaution I'd like to land it after we cut the RC so we have an opportunity to fix any regressions on a more relaxed timescale - button code is historically notorious for causing issues, even before recent changes.

@turt2live turt2live added the X-Blocked The PR cannot move forward in any capacity until an action is made label Apr 4, 2022
@luixxiul
Copy link
Contributor Author

luixxiul commented Apr 4, 2022

No problem about blocking until the next RC cut.

button code is historically notorious for causing issues, even before recent changes.

I would agree. As far as I checked, the code is quite complex and vulnerable to regressions.

@turt2live turt2live merged commit 35c49a8 into matrix-org:develop Apr 5, 2022
@luixxiul
Copy link
Contributor Author

luixxiul commented Apr 5, 2022

Thanks for checking- I'll keep an eye on a regression

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@turt2live turt2live turt2live approved these changes

Assignees
No one assigned
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks X-Blocked The PR cannot move forward in any capacity until an action is made
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@luixxiul @turt2live