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

MDLW: Modify multiselect dropdown widget to display selected pills. #19591

Closed
wants to merge 3 commits into from

Conversation

aryanshridhar
Copy link
Member

Commit 03e4fca - Revamps the multiselect dropdown list widget to display the selected dropdown pills beside the filter icon (Refer commit description for a better summary).
Commit befb74c - Removes the dead code arose due to the above change.

Screenshot
When implemented MDLW in Recent topic view (#19445) -

recent_topic_MDLW

@aryanshridhar aryanshridhar force-pushed the Refactor-MDLW branch 3 times, most recently from 0768f8d to bccbda6 Compare August 20, 2021 10:18
<div class="exit">
<span aria-hidden="true">&times;</span>
</div>
</div>
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

The name for this should be singular.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool, will fix it!

@timabbott
Copy link
Sponsor Member

@amanagr @sahil839 can one of you review and test this implementation?

I'm not convinced by the visuals for how selected items are indicated (the faded color seems wrong; I'd much prefer a checkmark + highlight) but that's probably just a CSS follow-up.

case "Escape": {
dropdown_menu.dropdown("toggle");
break;
}
Copy link
Collaborator

@sahil839 sahil839 Sep 20, 2021

Choose a reason for hiding this comment

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

This does not work correctly with bot-owner widget in the Change bot info and owner modal opened from settings, it closes the bot info modal. Also with message edit widget, Escape closes the edit form and not only the dropdown widget.

This commits essentially revamps the existing implementation of
Multiselect dropdown list widget to display pills beside the
dropdown for each selected item.

Along with the above change, here are the additional changes
included as well -

From UI standpoint -
- A funnel icon is displayed rather than the "No filters" button
  which toggles the dropdown.
- When any dropdown item is selected, the corresponding value is
  displayed in a pill next to the dropdown. Each pill has a "x" button
  next to it for cases when the user wants to remove them.
- Along with the above point, the dropdown item gets greyed out when
  pressed on it, indicating that it's pill value is already displayed.
- Removes the previously added "Filter" button, as now selecting a
  pill immediately applies it.

From code standpoint -
- A few parameters including `default_text`, `on_update` are removed
  as they are no longer required now.
- Introduced seperate handlebar templates for rendering MDLW widget
  and it's pill.
- The widget now takes `on_pill_add` and `on_pill_remove` parameters
  which trigger when a pill is added and removed respectively.
- Due to the above changes, modified the node tests for corresponding
  changes in dropdown_list_widget.
Within commit 7c588d4, we added an
function in list_widget.js which was used to retain a few dropdown
list items in case of MDLW.
However in previous commit, we revamped the Multiselect dropdown
list widget which eventually removes the use of above function.

This commit essentially removes the dead `retain_selected_items` function
in list_widget.js along with it's node tests as they are no longer required
now.
In commit 0d20a20, we missed out the "Escape" keyhandler
which is required to handle scenario where a user wants
to close the dropdown using "Escape" key.

This commit essentially handles the above case, which
enables the functionality to close dropdown using
"Esc" keyboard button.
@andersk
Copy link
Member

andersk commented Apr 28, 2022

(I rebased this to help along #21935, but I haven’t addressed any of the above feedback or made any other changes except updating it for refactors in main.)

@zulipbot
Copy link
Member

zulipbot commented Jul 7, 2022

Heads up @aryanshridhar, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

@timabbott
Copy link
Sponsor Member

Closing as we've deleted the original DropdownListWidget class in favor of a new dropdown_widget component based on Tippy.

@timabbott timabbott closed this Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants