-
Notifications
You must be signed in to change notification settings - Fork 38
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
Remove select2 and uses choices.js #4319
Conversation
d657275
to
e60e2a1
Compare
I made use of hypha/hypha/static_src/javascript/file-uploads.js Lines 1 to 30 in 5743656
|
…for choices initialisation/selection
e2c0acb
to
9f4ef09
Compare
@frjo It is ready for testing. I'll create a separate PR for the dynamicity of properties of the Choices element in base.html while removing similar code form other places like reviewers etc. |
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.
Works well in all the places I tested it, project table, meta terms, reminders and batch actions. The implementation look good as well, clean and neat.
Some comments on the styling of selects.
|
The implementation details looks much better now, neat and clean. Thanks for making that change. I've not checked for styling issues, yet. @frjo seems to have recommendation, plz consider using them or consider an alternative dropdown ui similar to how multiple select works in the submission all page. |
@frjo The above issues have been fixed, you may check it again. It is ready for testing. |
const minWidth = window.getComputedStyle(choiceElement).getPropertyValue("min-width"); | ||
choiceElement.addEventListener("focus", () => { | ||
if (choiceElement) { | ||
choiceElement.placeholder = "Search..." |
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.
Need to add something like choiceElement.style.minWidth = "10ch";
or the search placeholder gets cut of if the original placeholder is shorter.
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.
Right, 7ch looks enough for the search placeholder.
hypha/templates/base.html
Outdated
allowHTML: true, | ||
removeItemButton: true, | ||
}); | ||
selectElement.choicesInstance = choicesInstance; |
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.
Not enough to set selectElement.choicesInstance = true;
?
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.
Cleaner solution than my hack in any case.
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.
Yes, true should work. It will be more efficient.
Latest version deployed to test now. |
Fixes #4218
Test Steps