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

Scaling text input field to keep enough room for the buttons #10164

Merged
merged 1 commit into from
Feb 11, 2019
Merged

Scaling text input field to keep enough room for the buttons #10164

merged 1 commit into from
Feb 11, 2019

Conversation

4c0n
Copy link
Contributor

@4c0n 4c0n commented Feb 9, 2019

Q A
Branch? 1.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Related tickets
License MIT

When using some languages the buttons break out the search segment, because the text on them is longer than what can fit in the semantic ui determined column size.
This change makes sure that the buttons always get enough room, by scaling the size of the text input field. Without breaking the layout.

To test this or reproduce the original behavior:

  1. please add the Dutch language from the admin side. And assign it to the default channel.
  2. Add a translation for a product and for its taxon.
  3. Now if you go to the /nl/taxons/<dutch_taxon_slug> route you will find that the layout is broken.

@4c0n 4c0n requested a review from a team as a code owner February 9, 2019 23:24
@4c0n
Copy link
Contributor Author

4c0n commented Feb 10, 2019

In french it is even more noticable, so here are 2 screenshots to illustrate:
Before and after:
screenshot from 2019-02-10 11-51-25
screenshot from 2019-02-10 11-54-17

Copy link
Member

@Zales0123 Zales0123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm totally 👍 looking at the result (it's obviously better 😄), but I would also like to see the opinion of someone more fluent with CSS if we do not violent any convention/rules cc @kulczy

@kulczy
Copy link
Member

kulczy commented Feb 11, 2019

@4c0n thanks for that! Just a small tip,.ui.grid has already flex and for styling, let's try to use classes instead of id's

@4c0n
Copy link
Contributor Author

4c0n commented Feb 11, 2019

@Zales0123 Thanks!
@kulczy You're welcome. I'll try to remove the display: flex rule on that id.
Regarding the use of id's instead of classes. This is purposely done (and a commonly accepted way) to override framework CSS, as to solely apply it to these elements (specificity rules). I guess the other option would be to use !important on less specific CSS selector combinations, but that seems to be a worse solution. If you have another suggestion, please advise. Adding more classes would take away from Semantic UI, but might be possible if !important isn't used (or by using it, which I'd like to prevent at all costs, because of it being a bad practice).

@Zales0123 Zales0123 added the Shop ShopBundle related issues and PRs. label Feb 11, 2019
@kulczy
Copy link
Member

kulczy commented Feb 11, 2019

@4c0n I think the best option would be to extend semantic to handle auto-width columns, we'll probably do it in the future. And for now, your arguments sound good to me 👍

@pamil pamil merged commit 372a1b8 into Sylius:1.3 Feb 11, 2019
@pamil
Copy link
Contributor

pamil commented Feb 11, 2019

Thank you, Youri! 🥇

@4c0n
Copy link
Contributor Author

4c0n commented Feb 11, 2019

@kulczy Thanks! I agree extending Semantic UI would be awesome for this. I updated the PR (removing the redundant style), but it was already merged. Please reopen the PR if you still want to remove the redundancy.
@pamil Thanks for merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Shop ShopBundle related issues and PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants