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

Tiled gallery: gutter #14705

Closed
wants to merge 4 commits into from
Closed

Tiled gallery: gutter #14705

wants to merge 4 commits into from

Conversation

simison
Copy link
Member

@simison simison commented Feb 16, 2020

Add a gutter option to the Tiled gallery block.

image

Still little buggy and work in progress. Feel free to take PR over if you wanna help work on this!

Resolves #9548

Allows gutter options only as steps. The previous flexible solution I tried kinda would've exploded CSS bundle size.

Needs deciding styles for the gutter picker, two options I'm testing are select input and button group:

image

Latter gets little tricky with words translated sometimes longer in other languages:

image

Something compact like this could be an alternative but maybe less clear what those values are:

image

Changes proposed in this Pull Request:

  • Adds gutter option to Tiled gallery block's sidebar

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • Enhancement

Testing instructions:

  • Add tiled gallery block and some images
  • Change gutter in the sidebar for all different layouts
  • Test rescaling the page
  • Test different numbers of columns
  • Test with a different number of images
  • Test also the frontend of the site

@simison simison added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] In Progress [Block] Tiled Gallery labels Feb 16, 2020
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello simison! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D38976-code before merging this PR. Thank you!

@jetpackbot
Copy link

jetpackbot commented Feb 16, 2020

Warnings
⚠️ "Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 67f04e7

@simison
Copy link
Member Author

simison commented Feb 17, 2020

@iamtakashi since you've probably looked into quite a few block UIs like this, do you have thoughts on what would be good UI for changing the gutter setting? 🙂

@iamtakashi
Copy link
Contributor

@simison I think the common patterns that I've seen are either using a slider that gives you finer control or a dropdown that gives you presets like small, medium, etc. But I'd suspect it's hard to satisfy the user with the presets like that because those who care to change the gutter likely like to have finer control. What about a slider with 2px increments?

Screenshot 2020-02-17 at 15 24 24

@simison
Copy link
Member Author

simison commented Feb 17, 2020

@iamtakashi I agree slider would be the best — with the current technical implementation I just wanted to avoid creating too many allowed steps or otherwise the amount of CSS became crazy.

I do like the aspect that we could adjust named gutter sizes be different on different screen sizes. "Large" gutter on mobile and desktop is a very different thing in pixels. CoBlocks allows setting exact pixel size separately for desktop and mobile but while it's a lot of control, I feel like that's just confusing to most.

@iamtakashi
Copy link
Contributor

CoBlocks allows setting exact pixel size separately for desktop and mobile but while it's a lot of control, I feel like that's just confusing to most.

Responsive editing is being explored here: WordPress/gutenberg#19909. Until that's finalized, a single option for all the viewport should be fine?

I just wanted to avoid creating too many allowed steps or otherwise the amount of CSS became crazy.

I think 4px increment works as well. Would that also be too much CSS?

@simison
Copy link
Member Author

simison commented Feb 18, 2020

Responsive editing is being explored here: WordPress/gutenberg#19909. Until that's finalized, a single option for all the viewport should be fine?

Valid point — indeed better to stay away from creating custom UI for that stuff too early.

I just wanted to avoid creating too many allowed steps or otherwise the amount of CSS became crazy.

I think 4px increment works as well. Would that also be too much CSS?

What would be the maximum cap?

@iamtakashi
Copy link
Contributor

What would be the maximum cap?

How about 60px? A nice rounding number of a multiple of 4. So the range will be 0px to 60px, and the default is 4px as it is currently.

@simison
Copy link
Member Author

simison commented Mar 26, 2020

Haven't worked on this for a while but wanted to jot down some ideas.

One idea could be to apply a gutter class to each image, or even applying it as inline style. Might perform best and offer better flexibility. It could even not be stored in html output but applied on the fly via JS when doing layout resize-calculations.

@simison
Copy link
Member Author

simison commented Mar 26, 2020

CSS outputted via server-side as inline style would be effective but I really dislike making the block needlessly depend more on PHP.

@stale
Copy link

stale bot commented Jun 26, 2020

This PR has been marked as stale. This happened because:

  • It has been inactive in the past 3 months.
  • It hasn’t been labeled `[Pri] Blocker`, `[Pri] High`.

No further action is needed. But it's worth checking if this PR has clear testing instructions, is it up to date with master, and it is still valid. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.

@jeherve
Copy link
Member

jeherve commented Mar 2, 2022

I'll close this PR for now because of the lack of activity on this. We can always reopen in the future if needed, but it will need a rebase, so it may be easier to start a new PR at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Tiled Gallery Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Border Width for Tiled Galleries
6 participants