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

Convert "pullquote" design to feel more like a real pullquote. #2404

Merged
merged 3 commits into from Aug 15, 2017

Conversation

Projects
None yet
2 participants
@mtias
Contributor

mtias commented Aug 14, 2017

Wide:
image

Floated:
image

@jasmussen we need to clean up these floated styles that are starting to become awkward with overwrites.

@mtias mtias added the Blocks label Aug 14, 2017

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 14, 2017

Contributor

Try pulling. I think I fixed your floats. Let me know if they work as you intended.

Contributor

jasmussen commented Aug 14, 2017

Try pulling. I think I fixed your floats. Let me know if they work as you intended.

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Aug 14, 2017

Codecov Report

Merging #2404 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2404      +/-   ##
==========================================
- Coverage   25.91%   25.91%   -0.01%     
==========================================
  Files         157      157              
  Lines        4851     4859       +8     
  Branches      821      824       +3     
==========================================
+ Hits         1257     1259       +2     
- Misses       3034     3039       +5     
- Partials      560      561       +1
Impacted Files Coverage Δ
blocks/color-palette/index.js 0% <0%> (ø) ⬆️
blocks/library/cover-text/index.js 36% <0%> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77fa928...5ad3884. Read the comment docs.

codecov bot commented Aug 14, 2017

Codecov Report

Merging #2404 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2404      +/-   ##
==========================================
- Coverage   25.91%   25.91%   -0.01%     
==========================================
  Files         157      157              
  Lines        4851     4859       +8     
  Branches      821      824       +3     
==========================================
+ Hits         1257     1259       +2     
- Misses       3034     3039       +5     
- Partials      560      561       +1
Impacted Files Coverage Δ
blocks/color-palette/index.js 0% <0%> (ø) ⬆️
blocks/library/cover-text/index.js 36% <0%> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77fa928...5ad3884. Read the comment docs.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 14, 2017

Contributor

Also, I love this by the way! 👍 👍 on the design.

Contributor

jasmussen commented Aug 14, 2017

Also, I love this by the way! 👍 👍 on the design.

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Aug 14, 2017

Contributor

@jasmussen hah, no, I did the negative margin (shown on screenshot) on purpose, otherwise readability suffers:

image

Contributor

mtias commented Aug 14, 2017

@jasmussen hah, no, I did the negative margin (shown on screenshot) on purpose, otherwise readability suffers:

image

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 14, 2017

Contributor

Okay I'll look at doing the negative margin differently, and make sure there's a responsive play.

Contributor

jasmussen commented Aug 14, 2017

Okay I'll look at doing the negative margin differently, and make sure there's a responsive play.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 14, 2017

Contributor

So, as it turns out, we can't easily do "pullout" margins like you suggest here, without rewriting all of the alignment stuff we have here: https://github.com/WordPress/gutenberg/blob/master/editor/modes/visual-editor/style.scss#L159

We can do that, sure. But it's not pretty, and basically you'd end up with such exotic CSS that even on the front-end you'd want to make sure the theme could handle it. I.e. we're almost in add_theme_support territory here.

I'll be looking at separately improving the float situation, see if I can simplify things. I don't know that I can, but I'll look at it again. In the mean time I don't think this PR should be held off from reaching master without the "pullout" margins. What do you think?

Contributor

jasmussen commented Aug 14, 2017

So, as it turns out, we can't easily do "pullout" margins like you suggest here, without rewriting all of the alignment stuff we have here: https://github.com/WordPress/gutenberg/blob/master/editor/modes/visual-editor/style.scss#L159

We can do that, sure. But it's not pretty, and basically you'd end up with such exotic CSS that even on the front-end you'd want to make sure the theme could handle it. I.e. we're almost in add_theme_support territory here.

I'll be looking at separately improving the float situation, see if I can simplify things. I don't know that I can, but I'll look at it again. In the mean time I don't think this PR should be held off from reaching master without the "pullout" margins. What do you think?

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Aug 14, 2017

Contributor

I'll be looking at separately improving the float situation, see if I can simplify things. I don't know that I can, but I'll look at it again. In the mean time I don't think this PR should be held off from reaching master without the "pullout" margins. What do you think?

Maybe we can lower font-size slightly and the width of the pullquote when floated.

Contributor

mtias commented Aug 14, 2017

I'll be looking at separately improving the float situation, see if I can simplify things. I don't know that I can, but I'll look at it again. In the mean time I don't think this PR should be held off from reaching master without the "pullout" margins. What do you think?

Maybe we can lower font-size slightly and the width of the pullquote when floated.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 14, 2017

Contributor

I think that's fine.

Contributor

jasmussen commented Aug 14, 2017

I think that's fine.

@mtias mtias merged commit f1d9f67 into master Aug 15, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@mtias mtias deleted the update/make-pullquote-a-pullquote branch Aug 15, 2017

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