-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
[BUU] Fix Display ordering in shopfront field to allow re-ordering of the sequence #12860
[BUU] Fix Display ordering in shopfront field to allow re-ordering of the sequence #12860
Conversation
@@ -182,7 +182,6 @@ label .select2-container { | |||
.select2-container { | |||
.select2-choice, | |||
.select2-choices { | |||
height: $btn-relaxed-height !important; // !important is needed because of vendor/assets/stylesheets/select2.css.scss |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This rule was causing the choices to have a fixed height and hence the choices' height was not being adjusted as per the content in it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, I've always been against using height
like this!
I recall that JB was using height
to help line up elements in the new design, so it's possible that this will impact on the layout. I'll add a note for testers to check.
@@ -229,16 +228,14 @@ label .select2-container { | |||
display: flex; | |||
align-items: center; | |||
justify-content: center; | |||
padding-left: 7px; | |||
|
|||
.select2-search-choice-close { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stying the close button such that it's displayed with the icon-remove
. Currently, it's not displayed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to add an improvement while we're here.
But perhaps there's a reason for this being hidden. I don't think the shop owners have the ability to remove categories. So do the removed ones will get repopulated at the end of the list after saving? (I guess that's what happens when a new category is added).
Testers will probably pick this up, but it's probably worth considering the impact of this a bit more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the note @dacook. This PR just tweaks the styling, and the actual functionality and behavior are the same as that of the old UI.
Just to be sure, I validated all the cases of updating the category i.e. add, remove. Both cases are working as expected. When we remove the category, it gets removed as well.
So, I think it's safe to say now the page is exactly mimicking the old UI with new styling.
Please let me know if you have any questions on this one. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, but I'm not sure about the second part, so can you confirm my query before moving to testing?
@@ -182,7 +182,6 @@ label .select2-container { | |||
.select2-container { | |||
.select2-choice, | |||
.select2-choices { | |||
height: $btn-relaxed-height !important; // !important is needed because of vendor/assets/stylesheets/select2.css.scss |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, I've always been against using height
like this!
I recall that JB was using height
to help line up elements in the new design, so it's possible that this will impact on the layout. I'll add a note for testers to check.
@@ -229,16 +228,14 @@ label .select2-container { | |||
display: flex; | |||
align-items: center; | |||
justify-content: center; | |||
padding-left: 7px; | |||
|
|||
.select2-search-choice-close { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to add an improvement while we're here.
But perhaps there's a reason for this being hidden. I don't think the shop owners have the ability to remove categories. So do the removed ones will get repopulated at the end of the list after saving? (I guess that's what happens when a new category is added).
Testers will probably pick this up, but it's probably worth considering the impact of this a bit more.
@dacook - I've addressed your query here: #12860 (comment) Thanks. |
@@ -229,16 +228,14 @@ label .select2-container { | |||
display: flex; | |||
align-items: center; | |||
justify-content: center; | |||
padding-left: 7px; | |||
|
|||
.select2-search-choice-close { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @chahmedejaz, BeforeAfterProblems
I think we should fix this. It's working but very inconvenient currently. Moving back to In Progress. |
- disabled width on select2-search-choice-close so that it doesn't cover whole option
Thanks for raising these issues @drummer83, they needed to be fixed in this one. These issues have been fixed now. Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
i've checked that:
LGTM! Merging 🎉 thanks @chahmedejaz 💪 |
What? Why?
Before:
After:
What should we test?
Release notes
Changelog Category (reviewers may add a label for the release notes):