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

Reader: ensure button block matches main site styles #43779

Conversation

tophertoy
Copy link
Contributor

@tophertoy tophertoy commented Jun 29, 2020

Changes proposed in this Pull Request

This change handles styling the button block in reader to match the website It is only part 1 of 8 of this issue:

#43595

Testing instructions

Before:

Screenshot 2020-07-01 13 50 54

Taken from: http://calypso.localhost:3000/read/blogs/159889361/posts/1363

Firefox After:

Screenshot 2020-07-01 15 17 40

@tophertoy tophertoy requested a review from a team as a code owner June 29, 2020 21:13
@bluefuton bluefuton added [Status] In Progress [Feature] Reader The reader site on Calypso. labels Jun 29, 2020
@tophertoy
Copy link
Contributor Author

tophertoy commented Jun 30, 2020

@leandroalonso Here's what I have so far.

2020-06-30 14 38 14

Do you have thoughts on what styling you want brought over?

I'm told reader is opinionated on colours, so I haven't brought those over.

@tophertoy tophertoy force-pushed the fix/43595-button-block-styling-on-reader-matches-site branch from 8ec60b5 to 30d98fd Compare June 30, 2020 02:41
@leandroalonso
Copy link
Member

Hi @tophertoy! Thanks for your contribution!

@bluefuton would you mind reviewing this one?

@bluefuton
Copy link
Contributor

Thanks for the contribution @tophertoy! Can we match the styles applied to the default Calypso button?

https://wordpress.com/devdocs/packages/components/src/button/README.md?term=button

I think the 'non-primary' version would make the most sense. It looks like this:

Screen Shot 2020-07-01 at 11 29 27

@tophertoy
Copy link
Contributor Author

@bluefuton that link appears not to be working for me. Also https://wordpress.com/devdocs/ is 404ing.

Do you want me to work from the attached "Bananas" image?

@bluefuton
Copy link
Contributor

@tophertoy tophertoy force-pushed the fix/43595-button-block-styling-on-reader-matches-site branch from 30d98fd to f8bf89f Compare July 1, 2020 01:44
@tophertoy tophertoy changed the title WIP: Ensure button block on reader matches main site styles Ensure button block on reader matches main site styles Jul 1, 2020
@tophertoy
Copy link
Contributor Author

@bluefuton ok I've made some changes. Attached screenshots from firefox and chrome at the top.

Let me know what you think or if there is a defined browser list I should check.

@bluefuton bluefuton added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Jul 1, 2020
Copy link
Contributor

@bluefuton bluefuton left a comment

Choose a reason for hiding this comment

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

Looks good so far - just a couple of tweaks to bring it in line with the Button component.

client/blocks/reader-full-post/_content.scss Show resolved Hide resolved
client/blocks/reader-full-post/_content.scss Show resolved Hide resolved
@tophertoy
Copy link
Contributor Author

@bluefuton I've made those changes.

@bluefuton
Copy link
Contributor

Just one last tweak needed I think - let's add a touch of right and bottom margin to prevent multiple buttons running into each other:

Screen Shot 2020-07-01 at 15 44 53

@bluefuton bluefuton changed the title Ensure button block on reader matches main site styles Reader: ensure button block matches main site styles Jul 1, 2020
@tophertoy
Copy link
Contributor Author

tophertoy commented Jul 2, 2020

Gap is showing as 8px between buttons on right and below now:

Screenshot 2020-07-02 13 43 12

Screenshot 2020-07-02 13 43 01

Screenshot 2020-07-02 13 48 28

Thoughts?

Co-authored-by: Chris R <chris@bluefuton.com>
Copy link
Contributor

@bluefuton bluefuton left a comment

Choose a reason for hiding this comment

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

Looks good! 🚢

@bluefuton bluefuton merged commit d1e12d3 into Automattic:master Jul 2, 2020
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Reader The reader site on Calypso. OSS Citizen
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants