MozillaFoundation / foundation.mozilla.org

Mozilla Foundation website
https://foundation.mozilla.org
Mozilla Public License 2.0
388 stars 153 forks source link

[PNI] Collapse button of the sorting dropdown always resets your sorting preference #9763

Open mmmavis opened 1 year ago

mmmavis commented 1 year ago

Describe the bug

Clicking the collapse button of the sorting dropdown resets your sorting preference.

The arrow icon should only be used to open or close the dropdown and be independent of the sort option (currently they're linked) and changes the sort option by error when a user tries to close the dropdown.

To Reproduce

Steps to reproduce the behavior:

  1. Go to PNI homepage
  2. Click on the sorting dropdown and select something other than the default creepiness least - most
  3. Click on the sorting dropdown again and do not make any changes to your current sorting selection.
  4. Click on the "up arrow" button to close the dropdown
  5. Notice that hovering/clicking the "up arrow" button also triggers selection of creepiness least - most
  6. Products were sorted into creepiness least - most order without user's intention.

Expected behavior

Collapsing the sorting dropdown should not change the user's current sorting selection.

When clicking outside the box the dropdown collapses too.

Acceptance Criteria

┆Issue is synchronized with this Jira Bug

cdanfon commented 1 year ago

@beccaklam @nancyt1

Could you confirm what the expected behaviour should be based on Mavis's comment above please?

Do we need the option to close the dropdown without changing the selection? Should the dropdown close when we click outside the dropdown box too?

nancyt1 commented 1 year ago

The arrow icon should be used to open or close the dropdown and should not be changing the sort option or used to select any sort options. It would also be good to collapse the dropdown when you click outside of the box.

cdanfon commented 1 year ago

@fessehaye @danielfmiranda @tbrlpld

Nancy has confirmed behaviour, could one of you groom this ticket ahead of planning next week please? Thanks

@mtdenton 'cc

mmmavis commented 1 year ago

There are some other issues associated with the functionality part of this sorting dropdown (e.g., #10055). In order to cover them all, I want to propose that we refactor JS code of the dropdown and sorting related bits. For example, functions for sorting should be on its own and not embedded in event handler of the dropdown. For any sorting scenarios, whether it's through selection of the dropdown or on page load, we should be directly calling the sorting function instead of calling dropdown's click handler to trigger the sorting. 💬 <--- I took a closer look at the codebase and am now unsure if this example I gave was a good one. Crossing it out to avoid confusion.

I haven't got time to dive into the code more. Above is the issue that I discovered so far.

tbrlpld commented 1 year ago

Yup. Those are really good points @mmmavis

mmmavis commented 1 year ago

I took a closer look at the codebase yesterday and am now unsure if the example I gave in my comment above was a good one. I crossed it out to avoid confusion. There are multiple areas for improvement so we should still come up with a refactor/rewrite plan though.

mmmavis commented 1 year ago

I think I know what's causing this bug. Clicking on any option in the sorting dropdown collapses the dropdown. In the source code, the up arrow is just a SVG included as part of the first option. Itself is not implemented as a dropdown collapsing button. Clicking on the up arrow essentially means selecting the first option (Creepiness: Least - Most) hence creates the bug described in this ticket.

image

We should be able to fix this issue by

data-sync-user commented 3 months ago

➤ Mavis Ou commented:

We’ve learned that PNI tickets often ended up taking a lot longer time than anticipated to complete. It’s safe to overestimate the work needed for this ticket than underestimated 😅

data-sync-user commented 3 months ago

➤ Mavis Ou commented:

For this ticket, we should also add Playwright tests for the expected behaviours.