-
Notifications
You must be signed in to change notification settings - Fork 1
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
Some tweaks #52
Some tweaks #52
Conversation
@wilsonge Ready for review |
@@ -1,6 +1,6 @@ | |||
// Custom Forms | |||
|
|||
.form-select { | |||
.custom-select { |
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'd done this deliberately because of the var changes and thought that combined with this alias I would have things working :/ https://github.com/joomla/joomla-cms/pull/32037/files#diff-f81e8b4ffce389a8b743a7a2be34106f05ba6de248f9342471733c75cf919b7eR180
Why do we need this 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.
Yep you are right. I'd forgotten this was a bootstrap class. It seems custom-select
along with our own custom variants of this (eg. custom-select-color-state
) was still been set in all the xmls and a number of views. A blanket search and replace to form-select-*
seems to be working fine. I'll push it later this eve once I test a little further.
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.
Yeah I thought the @extend
would work to cover the majority of cases (at least in admin I think the native custom-select works - I'm just unsure about the nested properties) - I mean it's not too hard for global search/replace but assumed backporting it would be easier
@ciar4n thankyou so much for this! Other than trying to figure out the form-select/custom select thing - which is probably me doing dumb stuff - I think this looks fine! |
OK I've reverted the custom select for now so people can start testing what's there. Once you've figured out what's required for custom select just do a fresh pr here and i can merge. |
👍 Just note that with the revert the 'state' select fields (published/unpublished) will not change color. |
ack. We need to fix that. I'll have to debug in the browser. With the form-select class alias'd and not changing the class names for the state select fields I thought we'd have covered stuff. Ideally I don't want 3rd parties to have to change their code (I mean ideally we should alias it so there's the new and old way but without duplicate SCSS - just because the pressure is on from Production to reduce amount of work extension devs have to do) |
I'll submit a PR. I think in the views and XML we should use the BS5 |
Pull Request for Issue # .
Summary of Changes
Testing Instructions
Actual result BEFORE applying this Pull Request
Expected result AFTER applying this Pull Request
Documentation Changes Required