Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[amp story shopping] Remove multi line border radius code #37808

Closed
processprocess opened this issue Mar 2, 2022 · 0 comments · Fixed by #37817
Closed

[amp story shopping] Remove multi line border radius code #37808

processprocess opened this issue Mar 2, 2022 · 0 comments · Fixed by #37817

Comments

@processprocess
Copy link
Contributor

processprocess commented Mar 2, 2022

Description

Update:
Remove the multi line border radius code. It's creating motion inconsistency, giving uneven priority to the eye. It also does not provide utility, the two line text still renders with enough room.

Removing this will create visual consistency and remove code.

Spoke with @hongcatlover and they agree.

Mar-04-2022.10-32-39.mp4
The border radius work in #37186 regressed. Using [git bisect](https://git-scm.com/docs/git-bisect), the regression was introduced in #37195. It looks like selectors were updated in CSS but not in JS of that PR. Visual diff tests were down when this regression was introduced so a diff was not triggered.

The three css selectors in styleTagText_ do not match between css and JS.
Screen Shot 2022-03-02 at 9 22 59 AM

The null checks and optional chaining can be removed since the work in #37793 runs this logic after the tag is created.

Multi-line border radius prior to regression:
Screen Shot 2022-03-02 at 9 16 56 AM

Current:
Screen Shot 2022-03-02 at 9 17 54 AM

@processprocess processprocess added this to To do in wg-stories Shopping via automation Mar 2, 2022
@processprocess processprocess changed the title [amp story shopping] Multi line border radius is gone [amp story shopping] Remove multi line border radius code Mar 4, 2022
@jshamble jshamble moved this from To do to In progress in wg-stories Shopping Mar 7, 2022
@processprocess processprocess added this to To do in wg-stories Sprint via automation Mar 15, 2022
@processprocess processprocess moved this from To do to In progress in wg-stories Sprint Mar 15, 2022
wg-stories Sprint automation moved this from In progress to Done Mar 15, 2022
wg-stories Shopping automation moved this from In progress to Done Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants