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

Some tweaks #52

Merged
merged 10 commits into from
Jan 18, 2021
Merged

Some tweaks #52

merged 10 commits into from
Jan 18, 2021

Conversation

ciar4n
Copy link

@ciar4n ciar4n commented Jan 17, 2021

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

@ciar4n ciar4n changed the title WIP - some tweaks Some tweaks Jan 17, 2021
@ciar4n
Copy link
Author

ciar4n commented Jan 17, 2021

@wilsonge Ready for review

@@ -1,6 +1,6 @@
// Custom Forms

.form-select {
.custom-select {
Copy link
Owner

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?

Copy link
Author

@ciar4n ciar4n Jan 18, 2021

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.

Copy link
Owner

@wilsonge wilsonge Jan 18, 2021

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

@wilsonge
Copy link
Owner

@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!

@wilsonge wilsonge merged commit 62b7587 into wilsonge:bs5 Jan 18, 2021
@wilsonge
Copy link
Owner

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.

@ciar4n
Copy link
Author

ciar4n commented Jan 18, 2021

👍 Just note that with the revert the 'state' select fields (published/unpublished) will not change color.

@ciar4n ciar4n deleted the 2percent branch January 18, 2021 12:03
@wilsonge
Copy link
Owner

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)

@ciar4n
Copy link
Author

ciar4n commented Jan 18, 2021

I'll submit a PR. I think in the views and XML we should use the BS5 form-select. In the CSS we can style both so either form-select or custom-select will work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants