search

jakearchibald / svgomg

Web GUI for SVGO
https://jakearchibald.github.io/svgomg/
MIT License
5.8k stars 482 forks source link

Fix settings misalignment #351

Closed Graxi closed 3 years ago

Graxi commented 3 years ago

Fixes #307

netlify[bot] commented 3 years ago

✔️ Deploy Preview for svgomg ready!

🔨 Explore the source changes: 5fe55d6a2257c5120eb44023485a1a3bc1d596b7

🔍 Inspect the deploy log: https://app.netlify.com/sites/svgomg/deploys/616d3d936466450008a1e190

😎 Browse the preview: https://deploy-preview-351--svgomg.netlify.app/

XhmikosR commented 3 years ago

Shouldn't we wrap all items, though? It seems a little fragile to only do this for specific options only.

Graxi commented 3 years ago

This change feels a bit hacky to me, but it's not your fault, it's flexbox's fault. I avoid flexbox these days if I can, it just doesn't behave how I expect in cases like this, or it requires extra markup to do simple things 😄

Could you fix this by making each setting-item-toggle a two column grid? Also, make the gap between the switch and text a gap rather than a margin on the material-switch.

You should be able to revert the markup change to index.html.

Hi I have updated the PR and made setting-item-toggle two column structure. Would you like to review again? Let me know if further changes are needed please.

Graxi commented 3 years ago

Shouldn't we wrap all items, though? It seems a little fragile to only do this for specific options only.

Hmm what do you mean? I think I have wrapped all setting-item-toggle in index.html. Is there anywhere else?

jakearchibald commented 3 years ago

@XhmikosR

Shouldn't we wrap all items, though? It seems a little fragile to only do this for specific options only.

I'm not suggesting doing this for specific options, I'm suggesting doing it for all of them.

jakearchibald commented 3 years ago

@Graxi

Hi I have updated the PR and made setting-item-toggle two column structure.

I think there's been a mixup! The layout is still flexbox, and the HTML has still been modified.

Flexbox has a lot of confusing edge cases around this stuff, so I'm suggesting using grid instead.

As in, in the CSS, make each setting-item-toggle a two-column, one-row grid rather than a flexbox. Also, make the gap between the switch and text a gap rather than a margin on the material-switch.

Using grid means the HTML doesn't need to change at all, since it doesn't have these flexbox edge cases.

XhmikosR commented 3 years ago

I know you are but I thought the current patch doesn't do that, my bad.

Graxi commented 3 years ago

@Graxi

Hi I have updated the PR and made setting-item-toggle two column structure.

I think there's been a mixup! The layout is still flexbox, and the HTML has still been modified.

Flexbox has a lot of confusing edge cases around this stuff, so I'm suggesting using grid instead.

As in, in the CSS, make each setting-item-toggle a two-column, one-row grid rather than a flexbox. Also, make the gap between the switch and text a gap rather than a margin on the material-switch.

Using grid means the HTML doesn't need to change at all, since it doesn't have these flexbox edge cases.

@jakearchibald A little off the topic but IMHO Flexbox is still handy in this case. For example, it helps to do center alignment easily when the text has multiple lines. If we use two-column grid instead of the previous Flexbox, there is gonna be extra markup to deal with the center alignment of the two column. What I did is to wrap the elements and keep the existing styles.

jakearchibald commented 3 years ago

Flexbox is still handy in this case. For example, it helps to do center alignment easily when the text has multiple lines. If we use two-column grid instead of the previous Flexbox, there is gonna be extra markup to deal with the center alignment of the two column

It seems fine in https://github.com/jakearchibald/svgomg/pull/352. Am I missing something?

Graxi commented 3 years ago

Flexbox is still handy in this case. For example, it helps to do center alignment easily when the text has multiple lines. If we use two-column grid instead of the previous Flexbox, there is gonna be extra markup to deal with the center alignment of the two column

It seems fine in #352. Am I missing something?

Yeah it seems I misunderstood what you meant. Thanks for pointing it out