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.

Sign up for GitHub

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

Jump to bottom

Replace selectisize tom select #6323

Merged
merged 2 commits into from
Oct 31, 2024
Merged

Replace selectisize tom select #6323

merged 2 commits into from
Oct 31, 2024

Conversation

akatsoulas
Copy link
Collaborator

No description provided.

@akatsoulas akatsoulas requested a review from escattone October 30, 2024 13:15
@akatsoulas akatsoulas mentioned this pull request Oct 30, 2024
Closed
escattone
escattone reviewed Oct 30, 2024
View reviewed changes
Copy link
Contributor

@escattone escattone left a comment

Choose a reason for hiding this comment

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

❤️ it! Nice find! It looks great except for one thing. For me, the x of the clear button isn't visible (see below):

image

It looks to me like the SCSS for the clear and remove button plugins is being added to the screen.scss, so I'm confused why it isn't visible.
@akatsoulas akatsoulas force-pushed the replace-selectisize-tom-select branch from 92bfb90 to 3b08bf7 Compare October 31, 2024 11:45
akatsoulas
akatsoulas commented Oct 31, 2024
View reviewed changes
kitsune/sumo/static/sumo/js/wiki.editor.js
new TomSelect("select[id='id_restrict_to_groups']", {
closeAfterSelect: true,
plugins: {
clear_button: {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@escattone tom-select doesn't have an x for the clear button but rather a checkbox.
See https://tom-select.js.org/plugins/clear-button/

We could override this but unless there are strong objections I would prefer to tackle this in a different PR/issue to avoid scope creep. Thoughts?
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, let's tackle that in another PR/issue!

However, I do see a small x both in our HTML and on their website.
image
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that's for Bootstrap 4/5. If you select the Default from the drop down it's the checkbox..
escattone
escattone approved these changes Oct 31, 2024
View reviewed changes
Copy link
Contributor

@escattone escattone left a comment

Choose a reason for hiding this comment

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

r+wc
@escattone escattone merged commit 026d0a3 into mozilla:main Oct 31, 2024
2 checks passed
@akatsoulas akatsoulas deleted the replace-selectisize-tom-select branch October 31, 2024 14:42
@emilghittasv
Copy link
Collaborator

❤️ it! Nice find! It looks great except for one thing. For me, the x of the clear button isn't visible (see below):
image

It looks to me like the SCSS for the clear and remove button plugins is being added to the screen.scss, so I'm confused why it isn't visible.

I've also encountered this issue during testing in stage. Filled under mozilla/sumo#2029
@emilghittasv emilghittasv mentioned this pull request Nov 4, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants
@akatsoulas @emilghittasv @escattone