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

Adjust gallery caption flex alignment #9762

Merged
merged 5 commits into from Sep 13, 2018

Conversation

Projects
None yet
4 participants
@kjellr
Contributor

kjellr commented Sep 10, 2018

Gallery captions are meant to appear at the bottom of gallery items, but a recent change in #9622 changed that and introduced a visual bug (#9752). This sets things back to the way they were before.


Before:

screen shot 2018-09-10 at 1 17 12 pm

After:
screen shot 2018-09-10 at 1 14 16 pm


Fixes #9752

Adjust gallery caption flex alignment.
Gallery captions are meant to appear at the bottom of gallery items, but a recent change in #9622 changed that and introduced a visual bug. This sets things back to the way they were before.

Fixes #9752
@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 11, 2018

Contributor

Thanks so much for making this PR.

This is working well in Chrome:

screen shot 2018-09-11 at 08 44 04

Frontend:

screen shot 2018-09-11 at 08 44 10

Also working decently in Edge:

screen shot 2018-09-11 at 08 52 54

But it's not working well in IE with the "crop" option toggled off:

screen shot 2018-09-11 at 08 50 01

I'm going to take a look and see if I can fix this in your PR and push some things.

Contributor

jasmussen commented Sep 11, 2018

Thanks so much for making this PR.

This is working well in Chrome:

screen shot 2018-09-11 at 08 44 04

Frontend:

screen shot 2018-09-11 at 08 44 10

Also working decently in Edge:

screen shot 2018-09-11 at 08 52 54

But it's not working well in IE with the "crop" option toggled off:

screen shot 2018-09-11 at 08 50 01

I'm going to take a look and see if I can fix this in your PR and push some things.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 11, 2018

Contributor

Hmm looks like all those issues are unrelated to your work here. Will take another look.

Contributor

jasmussen commented Sep 11, 2018

Hmm looks like all those issues are unrelated to your work here. Will take another look.

jasmussen added some commits Sep 11, 2018

Fix issues with captions.
Props for the code to @webmandesign.
@jasmussen

I would love it if someone else had the time to test this after my pushes. @gziolo maybe?

But with the recent changes, (Thank you @webmandesign for your tip in #8594 (comment), I used it in bec52eb) issues with captions and cropping and your Chrome caption fix are addressed.

IE:

screen shot 2018-09-11 at 09 24 19

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 11, 2018

Contributor

This PR fixes #9764 and fixes #8594.

Contributor

jasmussen commented Sep 11, 2018

This PR fixes #9764 and fixes #8594.

@kjellr

This comment has been minimized.

Show comment
Hide comment
@kjellr

kjellr Sep 11, 2018

Contributor

This PR fixes #9764 and fixes #8594.

Confirmed. 👍 Thanks for the additions. I'll wait for one more set of eyes before we merge just to be safe.

Contributor

kjellr commented Sep 11, 2018

This PR fixes #9764 and fixes #8594.

Confirmed. 👍 Thanks for the additions. I'll wait for one more set of eyes before we merge just to be safe.

@jasmussen jasmussen requested a review from WordPress/gutenberg-core Sep 11, 2018

@jasmussen jasmussen added this to the 3.9 milestone Sep 11, 2018

@b3rend

This comment has been minimized.

Show comment
Hide comment
@b3rend

b3rend Sep 12, 2018

Did you guys also note about the horizontal alignment when cropping is disabled? I'm testing from 3.8.0-rc.1.

Editor Chrome:

hor_al

Front-end Chrome:

fr_chrome

b3rend commented Sep 12, 2018

Did you guys also note about the horizontal alignment when cropping is disabled? I'm testing from 3.8.0-rc.1.

Editor Chrome:

hor_al

Front-end Chrome:

fr_chrome

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 12, 2018

Contributor

Thanks for the report, @b3rend. I pushed a fix. Chrome:

screen shot 2018-09-12 at 12 06 33

Edge:

screen shot 2018-09-12 at 12 05 46

IE:

screen shot 2018-09-12 at 12 06 23

Yes, IE is not ideal. But it's as good as it's likely to get.

Contributor

jasmussen commented Sep 12, 2018

Thanks for the report, @b3rend. I pushed a fix. Chrome:

screen shot 2018-09-12 at 12 06 33

Edge:

screen shot 2018-09-12 at 12 05 46

IE:

screen shot 2018-09-12 at 12 06 23

Yes, IE is not ideal. But it's as good as it's likely to get.

@kjellr

This comment has been minimized.

Show comment
Hide comment
@kjellr

kjellr Sep 12, 2018

Contributor

The new changes are looking good. This fixes the caption position bug in the back end for IE11 very nicely, but it still looks a little bit off in the front end. Here you can see the captions overlapping other page content (in this case, the post meta):

screen shot 2018-09-12 at 12 04 41 pm

For reference, here's how that looks in the editor:

screen shot 2018-09-12 at 12 05 02 pm

(The fact that it's displayed as 2 columns in the editor seems like a weird separate bug — I'm able to reproduce that in master)

Otherwise, this looks great in both the front and back end of Chrome, FF, and Safari.

Contributor

kjellr commented Sep 12, 2018

The new changes are looking good. This fixes the caption position bug in the back end for IE11 very nicely, but it still looks a little bit off in the front end. Here you can see the captions overlapping other page content (in this case, the post meta):

screen shot 2018-09-12 at 12 04 41 pm

For reference, here's how that looks in the editor:

screen shot 2018-09-12 at 12 05 02 pm

(The fact that it's displayed as 2 columns in the editor seems like a weird separate bug — I'm able to reproduce that in master)

Otherwise, this looks great in both the front and back end of Chrome, FF, and Safari.

@kjellr

This comment has been minimized.

Show comment
Hide comment
@kjellr

kjellr Sep 12, 2018

Contributor

Setting bottom: 0 on the figcaption (as I did in 946844b) appears to fix that issue in IE11, and also has no ill effect on other browsers. It could use another set of 👀 though just to be sure.

Contributor

kjellr commented Sep 12, 2018

Setting bottom: 0 on the figcaption (as I did in 946844b) appears to fix that issue in IE11, and also has no ill effect on other browsers. It could use another set of 👀 though just to be sure.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 13, 2018

Contributor

Thanks so much for your help Kjell, this is awesome. We need this in 3.9.

Contributor

jasmussen commented Sep 13, 2018

Thanks so much for your help Kjell, this is awesome. We need this in 3.9.

@youknowriad

Confirmed, it works great for me 👍

@jasmussen jasmussen merged commit ebe0c82 into master Sep 13, 2018

1 check passed

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

@jasmussen jasmussen deleted the fix/gallery-caption-placement branch Sep 13, 2018

@kjellr

This comment has been minimized.

Show comment
Hide comment
@kjellr

kjellr Sep 13, 2018

Contributor
Contributor

kjellr commented Sep 13, 2018

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