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.

Sign up for GitHub

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

Jump to bottom

Fix link-in-bio styles. #79756

Merged
merged 4 commits into from
Jul 25, 2023
Merged

Fix link-in-bio styles. #79756

merged 4 commits into from
Jul 25, 2023

Conversation

allilevine
Copy link
Member

@allilevine allilevine commented Jul 21, 2023 •
edited
Loading

Issue raised here: p1689919397847559-slack-C9EJ7KSGH

Proposed Changes

  • Add the link-in-bio class to this giant list of style corrections. If these are needed for every flow, we should make them less specific.
  • Fix font for all expanding textareas.

Testing Instructions

  • Go to /setup/link-in-bio/linkInBioSetup
Before After
Screen Shot 2023-07-21 at 1 56 17 PM Screen Shot 2023-07-21 at 1 55 56 PM

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • Have you written new tests for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-ajp-p2)?

@github-actions
Copy link

github-actions bot commented Jul 21, 2023 •
edited by matticbot
Loading

Calypso Live (direct link)
https://calypso.live?image=registry.a8c.com/calypso/app:build-74960
Jetpack Cloud live (direct link)
https://calypso.live?image=registry.a8c.com/calypso/app:build-74960&env=jetpack

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

allilevine
allilevine commented Jul 21, 2023
View reviewed changes
client/landing/stepper/declarative-flow/internals/global.scss
@@ -77,6 +77,7 @@ button {
.domains,
.patterns,
.link-in-bio-post-setup,
.link-in-bio,
Copy link
Member Author

Choose a reason for hiding this comment

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

Adding this to cover /setup/link-in-bio/linkInBioSetup

allilevine
allilevine commented Jul 21, 2023
View reviewed changes
...landing/stepper/declarative-flow/internals/steps-repository/components/setup-form/style.scss
@@ -22,7 +22,8 @@
}

label,
textarea {
textarea,
.expanding-area textarea {
Copy link
Member Author

Choose a reason for hiding this comment

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

/setup/link-in-bio/linkInBioSetup has a specific .expanding-area textarea class that gets overwritten by another font family. I don't see it testing locally, but I see it on calypso.live.

@allilevine allilevine marked this pull request as ready for review July 21, 2023 18:26
@allilevine allilevine requested a review from edanzer July 21, 2023 18:26
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 21, 2023
@lsl
Copy link
Contributor

lsl commented Jul 24, 2023

This looks fixed in prod. Maybe someone else picked it up?

@allilevine
Copy link
Member Author

This looks fixed in prod. Maybe someone else picked it up?

I'm not seeing the fix on prod or staging. 🤔 I'm testing in Firefox and Chrome.

@lsl
Copy link
Contributor

lsl commented Jul 25, 2023

The first step is fine, the second one is not 🤦

nuriapenya
nuriapenya requested changes Jul 25, 2023
View reviewed changes
Copy link

@nuriapenya nuriapenya left a comment

Choose a reason for hiding this comment

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

The broken alignment seems to be fixed, but I still see issues with the styles of the buttons. In both Intro and Setup pages, the primary CTA still has the wrong values:

Perhaps this is more related to: https://github.com/Automattic/dotcom-forge/issues/3163

Screenshot 2023-07-25 at 08 10 33 Screenshot 2023-07-25 at 08 10 42

@nuriapenya
Copy link

Another thing I spot on the Launchpad screen in the testing link, is that the Header logo is repeated. Not happening in production.

Screenshot 2023-07-25 at 08 23 56

@lsl
Copy link
Contributor

lsl commented Jul 25, 2023 •
edited
Loading

Another thing I spot on the Launchpad screen in the testing link, is that the Header logo is repeated. Not happening in production.

Are we loading the header twice and hiding one? (I think I've seen this before somewhere, sorry couldn't find a link). I don't think this will be an issue in production, from memory it was some weird css ordering issue that only took place in development.

but I still see issues with the styles of the buttons.

This should be fixed by #79708.

lsl
lsl approved these changes Jul 25, 2023
View reviewed changes
Copy link
Contributor

@lsl lsl left a comment

Choose a reason for hiding this comment

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

LGTM

@paulopmt1
Copy link
Contributor

paulopmt1 commented Jul 25, 2023 •
edited
Loading

is that the Header logo is repeated. Not happening in production.

I was also seeing that in both dev and staging. Since it wasn't present on trunk, I fixed with this commit.

paulopmt1
paulopmt1 approved these changes Jul 25, 2023
View reviewed changes
Copy link
Contributor

@paulopmt1 paulopmt1 left a comment

Choose a reason for hiding this comment

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

Nice work @allilevine, it's working as expected!

@paulopmt1 paulopmt1 merged commit 013b94b into trunk Jul 25, 2023
@paulopmt1 paulopmt1 deleted the fix/link-in-bio-style branch July 25, 2023 14:19
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 25, 2023
@allilevine
Copy link
Member Author

Thank you! 🙌 Looks like everything except the button color has been resolved in production.

@paulopmt1
Copy link
Contributor

Thank you! 🙌 Looks like everything except the button color has been resolved in production.

I'll monitor #79708 and if it does not solve it, will create a fix for that.

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

@nuriapenya nuriapenya nuriapenya requested changes

@paulopmt1 paulopmt1 paulopmt1 approved these changes

@lsl lsl lsl approved these changes

@edanzer edanzer Awaiting requested review from edanzer

@andres-blanco andres-blanco Awaiting requested review from andres-blanco

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@allilevine @matticbot @lsl @nuriapenya @paulopmt1