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

Suggested styles #7

Closed
wants to merge 4 commits into from
Closed

Conversation

AaronRutley
Copy link

I've suggested some styles to improve the 'gutenberg-demo' post.

Copy link
Member

@ntwb ntwb left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@tyrann0us tyrann0us left a comment

Choose a reason for hiding this comment

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

Nit: Missing whitespace.

style.css Outdated
@@ -819,3 +819,7 @@ object {
.wp-block-embed {
text-align: center;
}

.wp-block-embed .twitter-tweet-rendered {
margin:0 auto;

Choose a reason for hiding this comment

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

Nit: Missing whitespace between property and value?


.wp-block-button.aligncenter {
display: block;
margin:0 auto;

Choose a reason for hiding this comment

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

Nit: Missing whitespace between property and value?

Copy link
Member

Choose a reason for hiding this comment

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

Ha, I missed that 😏

Copy link
Collaborator

Choose a reason for hiding this comment

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

More indentation here than in the rest of the stylesheet — maybe swap out to match the rest of the stylesheet?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at the demo, I don't think this should be full-width.

@ntwb ntwb requested a review from melchoyce August 17, 2017 07:47
.wp-block-button.aligncenter {
display: block;
margin:0 auto;
text-align: center;
Copy link
Collaborator

Choose a reason for hiding this comment

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

More indentation here than in the rest of the stylesheet — maybe swap out to match the rest of the stylesheet?

}

.wp-block-embed .twitter-tweet-rendered {
margin:0 auto;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs a space between margin and 0

@@ -30,7 +33,13 @@ ol,
.wp-block-cover-text.alignfull,
.wp-block-image.alignfull,
.wp-block-gallery.alignfull,
..wp-block-text-columns.alignfull{
.wp-block-text-columns.alignfull{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs a space between .alignfull and {

Copy link
Collaborator

@melchoyce melchoyce left a comment

Choose a reason for hiding this comment

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

Some minor spacing issues, but otherwise looking good — if you patch those up, and fix up the button width, I think it'll be good to go.


.wp-block-button.aligncenter {
display: block;
margin:0 auto;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at the demo, I don't think this should be full-width.

@karmatosed
Copy link
Member

Can you redo this PR with the suggestions in place please @AaronRutley? Note, there have been updates to the theme so make sure you grab the latest and resolve conflicts. Thanks!

@karmatosed
Copy link
Member

Closing as we have conflicts and have split this from the .org site now.

@karmatosed karmatosed closed this Jan 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants