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

Figure caption width in tutorial #1766

Conversation

jpeterson976
Copy link
Contributor

Fixes #1762

I may have added a little too much to the new tutorial section, so let me know if I should cut it back at all.

I also changed max-width to text-width since that's a little more accurate, and made a minor CSS change for text width captions to better account for medium and large sizes.

@jpeterson976 jpeterson976 requested review from a team, iturgeon and maufcost and removed request for a team April 7, 2021 19:39
@iturgeon iturgeon added this to the 20 - Tanzanite milestone Apr 7, 2021
@iturgeon iturgeon changed the base branch from dev/19-serendibite to dev/20-tanzanite April 7, 2021 20:36
@iturgeon iturgeon changed the base branch from dev/20-tanzanite to dev/19-serendibite April 7, 2021 20:39
@iturgeon iturgeon requested a review from zachberry April 7, 2021 20:49
@iturgeon
Copy link
Member

iturgeon commented Apr 7, 2021

The only argument I have against Text-width is when using captions on large images that extend past the text width. The image below shows the text width vs caption width under a large figure

Screen Shot 2021-04-07 at 4 53 59 PM

@zachberry
Copy link
Member

@jpeterson976 @iturgeon Both good points. max-width is used for Iframes for sizing which means beyond text width, so using max-width for figure width is probably a mistake. But there is the weird nuance that Ian brought up that text-width really means max-width when the figure size is large (In reality it seems there's no major difference for captionWidth when the image size is large).

Here's my thought - what if we stick with this change - text-width. But then disable the caption width select in the UI if the size is Large, and make a mention in the docs that this property has no effect if size is Large.

@zachberry
Copy link
Member

Actually, it doesn't have any effect for size = medium either, since medium images already are at text width. So perhaps we disable the input for large and medium sizes.

* Forces caption width to image-width when size is large or medium
@zachberry zachberry requested a review from a team April 8, 2021 15:42
@jpeterson976
Copy link
Contributor Author

Works great in the editor with my testing. Code looks good too, found some stuff I missed in the original PR. Only change I can think of is the new tutorial section should probably be updated to mention the caption width option is only available for small and custom sizes.

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.

3 participants