-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
[B2B] 284 - Blank Theme multiselect arrows are too small #30028
[B2B] 284 - Blank Theme multiselect arrows are too small #30028
Conversation
Hi @joctabio. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
You can find more information about the builds here ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review. For more details, please, review the Magento Contributor Guide documentation. 🕙 You can find the schedule on the Magento Community Calendar page. 📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket. 🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel ✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel |
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 @joctabio those changes are coming from B2B version.
I believe that same issue can popup also on community cause its a UI element. But please do not create Magento_B2b folder for fixing it. Better provide general fix for such UI element independent from Magento edition
Issue: https://github.com/magento/partners-magento2b2b/issues/284 @joanhe @danmooney2 what do you think would be the better approach on this one? |
@nuzil The problem component was introduced in the last B2B release. It is only available in B2B. The related theme issue should be addressed in B2B scope. |
…too small - Fix arrow down icon size of new multi-select
f0d1578
to
9e81d9a
Compare
@magento run all tests |
@joanhe the blank theme design folder of CE repo contains designs for EE modules, like Rma or GiftCardAccount. There are no restrictions on adding non-public modules design content to CE. Do you know are there any specific restrictions for B2B edition? |
@magento give me test instance |
Hi @nuzil. Thank you for your request. I'm working on Magento instance for you. |
Hi @nuzil, here is your Magento Instance: https://287e93edcc683850dc54a619653b8b82.instances.magento-community.engineering |
@_triangle__width: @button-marker-triangle__width; | ||
|
||
// Action select have the same visual styles and functionality as native <select> | ||
.admin__action-group-wrap { |
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 am not sure its correct. How this selector is going to fix Frontend styles?
Sorry I cannot reproduce it really on my instance, maybe you can give some overview.
@rganin btw on your screenshot is still not ok :-) |
align-items: center; | ||
content: @icon-down; | ||
display: flex; | ||
font-size: 24px; |
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.
@nuzil as you can see, I have set it to 24px
to make it work specifically for the Blank theme
since it has a significantly smaller font-icon asset for the arrow down.
In (B2B repo, 1.2-develop):
app/code/Magento/B2b/view/frontend/web/css/source/actions/_actions-select.less
the icon is set to 10px
which applies to any theme but only works perfectly for Luma
.
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.
Thats clear, thanks, but I mean this CSS selector is located in Admin:
" .admin__action-group-wrap {" or?
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.
@nuzil yes, the selector is targeting elements with admin__
prefixed class names as seen in the main module B2B 1.2-develop: app/code/Magento/B2b/view/frontend/web/css/source/actions/_actions-select.less
.
If you are recommending to refactor the CSS selector that was used in the storefront, then that would result in refactoring the template file as well which is located at:
B2B 1.2-develop: app/code/Magento/B2b/view/frontend/web/template/form/element/ui-group.html
that has all the existing admin__
prefixed class names.
@joanhe @rganin do you think this is still in scope with the issue and should we refactor the template file?
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.
@joctabio @nuzil discussed this with UI team members, @eug123 : This prefix does not have a functional purpose and is just a refactoring leftover that has not been eliminated due to priority changes. It exists in backend styles and has migrated to storefront with components like app/code/Magento/CheckoutAddressSearch/view/frontend/web/template/ui-select.html
, the subject one and some other. It is also used in some frontend styles, like app/design/frontend/Magento/luma/web/css/source/_forms.less
and others.
Blind removal is unapplicable here, some styles are common for storefront and backend and located in base
, also there is a non-zero probability of having style conflicts after prefix removal. This should be done in a separate story with comprehensive UI testing.
I suggest to leave it as is like in the mentioned template for now.
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.
Ok. Unfortnatelly I cannot check the UI myself, so UI tests are required. But code looks fine for me
Hi @nuzil, thank you for the review.
|
Hi @joctabio, thank you for your contribution! |
Description (*)
Fix new multi-select arrow down icon size for blank theme in Approval Rules page.
Fixed Issues
Manual testing scenarios (*)
Specific roles
optionContribution checklist (*)