Fixes a broken link and set margin around links on authentication dialog #8208

luixxiul · 2022-03-31T13:39:27Z

Add margin around links
Fix the bug that Go back link is enabled over the row
Also add the role to the error div

type: defect

Signed-off-by: Suguru Hirahara luixxiul@users.noreply.github.com

Here's what your changelog entry will look like:

🐛 Bug Fixes

Fixes a broken link and set margin around links on authentication dialog (#8208). Fixes Link placement on authentication dialog should be fixed element-hq/element-web#21624. Contributed by @luixxiul.

Preview: https://pr8208--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.

Closes element-hq/element-web#21624 Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>

codecov · 2022-03-31T13:50:21Z

Codecov Report

Merging #8208 (53eb605) into develop (942ca74) will decrease coverage by 0.92%.
The diff coverage is 50.00%.

❗ Current head 53eb605 differs from pull request most recent head c9e25a9. Consider uploading reports for the commit c9e25a9 to get more accurate results

@@             Coverage Diff             @@
##           develop    #8208      +/-   ##
===========================================
- Coverage    30.74%   29.82%   -0.93%     
===========================================
  Files          892      875      -17     
  Lines        50681    50024     -657     
  Branches     12900    12727     -173     
===========================================
- Hits         15584    14918     -666     
- Misses       35097    35106       +9

Impacted Files	Coverage Δ
src/components/views/auth/CaptchaForm.tsx	`4.34% <ø> (ø)`
src/components/structures/auth/Registration.tsx	`29.26% <50.00%> (-0.91%)`	⬇️
...list/algorithms/tag-sorting/AlphabeticAlgorithm.ts	`0.00% <0.00%> (-100.00%)`	⬇️
src/utils/colour.ts	`0.00% <0.00%> (-94.12%)`	⬇️
src/autocomplete/EmojiProvider.tsx	`16.27% <0.00%> (-72.86%)`	⬇️
...c/stores/room-list/algorithms/tag-sorting/index.ts	`14.28% <0.00%> (-71.43%)`	⬇️
src/components/views/elements/FacePile.tsx	`10.00% <0.00%> (-67.78%)`	⬇️
src/autocomplete/AutocompleteProvider.tsx	`0.00% <0.00%> (-60.72%)`	⬇️
...list/algorithms/list-ordering/OrderingAlgorithm.ts	`0.00% <0.00%> (-57.15%)`	⬇️
...stores/room-list/algorithms/list-ordering/index.ts	`14.28% <0.00%> (-57.15%)`	⬇️
... and 105 more

kerryarchibald · 2022-04-01T07:17:31Z

src/components/structures/auth/Registration.tsx

@@ -614,8 +614,8 @@ export default class Registration extends React.Component<IProps, IState> {
                    onServerConfigChange={this.state.doingUIAuth ? undefined : this.props.onServerConfigChange}
                />
                { this.renderRegisterComponent() }
-                { goBack }
-                { signIn }
+                <div className="mx_AuthBody_link_goBack">{ goBack }</div>


The margins of mx_Dialog_buttons_items will still be applied even when goBack is falsy, creating a weird gap in the UI when we don't render a go back button.

I guess you are talking about #8160?

kerryarchibald · 2022-04-01T07:19:49Z

src/components/structures/auth/Registration.tsx

-                { goBack }
-                { signIn }
+                <div className="mx_AuthBody_link_goBack">{ goBack }</div>
+                <div className="mx_AuthBody_link_signIn">{ signIn }</div>


The margin styles you define for these wrappers can be added to the style of the signIn and goBack elements themselves - both use the class mx_AuthBody_changeFlow
It's unnecessary to introduce more wrapping elements.

~~OK, I am going to fix the bug of the link with specifying additional rules. Actually mx_AuthBody_changeFlow of those elements do nothing here.~~ You need a wrapper anyway, in order to place an inlined element at the center. There could be another way of achieving that, but it needs an unnecessarily complicated solution.

Addressed: dbdf83d

Addressed: ca411f7.

TODO: margin setting. See below.

kerryarchibald · 2022-04-01T07:20:07Z

res/css/views/auth/_AuthBody.scss

+    }
+
+    .mx_AuthBody_link_goBack {
+        margin: 12px auto 6px;


Please use the spacing variables for standard spacings https://github.com/matrix-org/matrix-react-sdk/blob/develop/res/css/_spacing.scss

kerryarchibald · 2022-04-01T07:21:08Z

res/css/views/auth/_AuthBody.scss

@@ -94,6 +94,22 @@ limitations under the License.
            background-color: $authpage-focus-bg-color;
        }
    }
+
+    .mx_recaptchaContainer {
+        .mx_recaptchaContainer_info,


mx_recaptchaContainer_info is a specific selector, it does not need to be nested.

Generally semantic specificity does not prevent unexpected visual regressions. Nesting should. It also should serve its function not only on large SASS files but also on tiny ones, as checking visual regressions for the latter is a difficult task if there is not an automated test available. Setting an insurance is better than leaving them vulnerable to possibility of the regressions. If you don't have a strong opinion against it, I would rather keep this nested until another element outside of it requires this.

kerryarchibald · 2022-04-01T07:25:04Z

src/components/views/auth/CaptchaForm.tsx

                    { this.state.errorText }
                </div>
            );
        }

        return (
-            <div ref={this.recaptchaContainer}>
-                <p>{ _t(
+            <div className="mx_recaptchaContainer" ref={this.recaptchaContainer}>


We prefix our classes with the component they are styling - so the classes you've added should be mx_CaptchaForm_recaptchaContainer, mx_CaptchaForm_recaptchaContainerInfo etc
This is explained a bit more in the css rules in the developer guide https://github.com/matrix-org/matrix-react-sdk/blob/develop/README.md#developer-guide

Thanks for letting me know about that guide.

Addressed: 9425477

kerryarchibald · 2022-04-01T07:29:38Z

res/css/views/auth/_AuthBody.scss

@@ -94,6 +94,22 @@ limitations under the License.
            background-color: $authpage-focus-bg-color;
        }
    }
+
+    .mx_recaptchaContainer {


Components should have their own style files, since these classes are used in CaptchaForm component they should be in _CaptchaForm.scss. This helps keep our components independent and maintainable.

Please move these into the correct file (and don't forget to run yarn rethemendex when adding new scss files)

Addressed: 42874a9

As long as mx_AuthBody_changeFlow is specified to the AccessibleButton, the link may not be inlined as expected because of mx_AuthBody_changeFlow's styles (display: block & width: 100%). We need a wrapper anyway. Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>

mx_CaptchaForm_* Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>

Setting "p" tag instead of "mx_CaptchaForm_recaptchaContainer_info" would be too broad. Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>

Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>

luixxiul · 2022-04-10T04:12:09Z

I am having trouble of comprehending the usage of the spacing variables.

For which properties should the variables be used? Padding inside of an element? Margin between elements?
Should the variables be used as value of gap too?
Should spacing be done only with the absolute unit px as specified in _spacing.scss? Is there not a case where relative units such as rem would be preferred?

I also would like to know where the standardized usage of them is published and the guideline and documentation about it are available.

Answers would be very appreciated. Thank you in advance.

Edit: I've accustomed to the usage of the variables, so it is no longer a problem for me, while it does not mean the style guide about the usage is unnecessary.

Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>

luixxiul · 2022-05-27T08:03:11Z

Please review this one. thanks.

andybalaam · 2022-05-27T09:40:10Z

@kerryarchibald

luixxiul · 2022-06-16T05:36:46Z

@turt2live If this PR is not really necessary, please let me know...

janogarcia

@turt2live @luixxiul Yeah, let's please close this PR. It seems to have been addressed already in recent updates.

Screenshot from production app:

Fixes a broken link and set margin around links

f442200

Closes element-hq/element-web#21624 Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>

luixxiul requested a review from a team as a code owner March 31, 2022 13:39

luixxiul changed the title ~~Fixes a broken link and set margin around links~~ Fixes a broken link and set margin around links on authentication dialog Mar 31, 2022

github-actions bot added the T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems label Mar 31, 2022

kerryarchibald requested a review from a team March 31, 2022 16:14

kerryarchibald suggested changes Apr 1, 2022

View reviewed changes

luixxiul mentioned this pull request Apr 1, 2022

Fix visual bugs on AccessSecretStorageDialog #8160

Merged

luixxiul added 8 commits April 1, 2022 16:37

Add the prefix first

9425477

mx_CaptchaForm_* Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>

Create _CaptchaForm.scss

42874a9

Setting "p" tag instead of "mx_CaptchaForm_recaptchaContainer_info" would be too broad. Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>

mx_AuthBody_changeFlow_signIn

ca411f7

Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>

Remove mx_AuthBody_link_signIn

69bfbb7

Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>

Merge branch 'develop' into Captcha

8886568

Merge branch 'develop' into Captcha

51051e8

Merge branch 'develop' into Captcha

854a121

Merge branch 'develop' into Captcha

fee268b

luixxiul added 3 commits April 13, 2022 06:43

Merge branch 'develop' into Captcha

3c18ffe

Merge branch 'develop' into Captcha

53eb605

Merge branch 'develop' into Captcha

c9e25a9

t3chguy requested a review from kerryarchibald May 9, 2022 06:08

luixxiul added 2 commits May 13, 2022 02:48

Use a variable

b7151e5

Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>

Merge branch 'develop' into Captcha

feb3e48

github-actions bot deployed to Netlify May 12, 2022 18:01 View deployment

luixxiul added 2 commits May 16, 2022 04:27

Merge branch 'develop' into Captcha

9fe38a8

Merge branch 'develop' into Captcha

78d126b

github-actions bot deployed to Netlify May 17, 2022 10:43 View deployment

Merge branch 'develop' into Captcha

ee612e6

github-actions bot deployed to Netlify May 18, 2022 12:15 View deployment

luixxiul added 2 commits May 29, 2022 16:31

Merge branch 'develop' into Captcha

5ebe355

Merge branch 'develop' into Captcha

f6a8864

github-actions bot deployed to Netlify May 30, 2022 12:59 View deployment

Merge branch 'develop' into Captcha

d301634

MadLittleMods added the Z-Community-PR Issue is solved by a community member's PR label Jun 1, 2022

Merge branch 'develop' into Captcha

c24adb4

github-actions bot deployed to Netlify June 2, 2022 02:55 View deployment

luixxiul added 2 commits June 5, 2022 02:15

Merge branch 'develop' into Captcha

3282eec

Merge branch 'develop' into Captcha

1cd9ecc

github-actions bot deployed to Netlify June 6, 2022 02:02 View deployment

Merge branch 'develop' into Captcha

4d466e2

github-actions bot deployed to Netlify June 7, 2022 11:28 View deployment

Merge branch 'develop' into Captcha

c329107

github-actions bot deployed to Netlify June 8, 2022 04:31 View deployment

Merge branch 'develop' into Captcha

1a08e52

github-actions bot deployed to Netlify June 9, 2022 13:31 View deployment

Merge branch 'develop' into Captcha

3a986d1

github-actions bot deployed to Netlify June 11, 2022 08:39 View deployment

Merge branch 'develop' into Captcha

f18db65

github-actions bot deployed to Netlify June 15, 2022 15:20 View deployment

Merge branch 'develop' into Captcha

123ffc8

github-actions bot deployed to Netlify June 16, 2022 01:56 View deployment

Merge branch 'develop' into Captcha

ac783d7

github-actions bot deployed to Netlify June 16, 2022 16:15 View deployment

janogarcia reviewed Jun 17, 2022

View reviewed changes

luixxiul closed this Jun 17, 2022

luixxiul deleted the Captcha branch June 17, 2022 08:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes a broken link and set margin around links on authentication dialog #8208

Fixes a broken link and set margin around links on authentication dialog #8208

luixxiul commented Mar 31, 2022 •

edited by github-actions bot

Loading

codecov bot commented Mar 31, 2022 •

edited

Loading

kerryarchibald Apr 1, 2022

luixxiul Apr 1, 2022

kerryarchibald Apr 1, 2022

luixxiul Apr 1, 2022 •

edited

Loading

luixxiul Apr 1, 2022

luixxiul Apr 1, 2022

kerryarchibald Apr 1, 2022

kerryarchibald Apr 1, 2022

luixxiul Apr 1, 2022 •

edited

Loading

kerryarchibald Apr 1, 2022

luixxiul Apr 1, 2022

luixxiul Apr 1, 2022

kerryarchibald Apr 1, 2022

luixxiul Apr 1, 2022

luixxiul commented Apr 10, 2022 •

edited

Loading

luixxiul commented May 27, 2022

andybalaam commented May 27, 2022

luixxiul commented Jun 16, 2022

janogarcia left a comment

Fixes a broken link and set margin around links on authentication dialog #8208

Fixes a broken link and set margin around links on authentication dialog #8208

Conversation

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

🐛 Bug Fixes

codecov bot commented Mar 31, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luixxiul Apr 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luixxiul Apr 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luixxiul commented Apr 10, 2022 • edited Loading

luixxiul commented May 27, 2022

andybalaam commented May 27, 2022

luixxiul commented Jun 16, 2022

janogarcia left a comment

Choose a reason for hiding this comment

luixxiul commented Mar 31, 2022 •

edited by github-actions bot

Loading

codecov bot commented Mar 31, 2022 •

edited

Loading

luixxiul Apr 1, 2022 •

edited

Loading

luixxiul Apr 1, 2022 •

edited

Loading

luixxiul commented Apr 10, 2022 •

edited

Loading