search

hicommonwealth / commonwealth

A platform for decentralized communities
https://commonwealth.im
GNU General Public License v3.0
64 stars 38 forks source link

New Sort-Implementation + Filter UI for threads #3926

Closed mzparacha closed 11 months ago

mzparacha commented 1 year ago

Link to Issue

Closes: https://github.com/hicommonwealth/commonwealth/issues/3821

Description of Changes

image

Test Plan

Deployment Plan

N/A

Other Considerations

N/A

mzparacha commented 1 year ago

@jnaviask Almost ready. These are left

- Update thread state when new filters are applied - Polish new ui for pixel level updates

mzparacha commented 1 year ago

For > 921px filter responsive size, asked a question here

Update: This was resolved, Ty @meeshlin!

mzparacha commented 1 year ago

@jnaviask Ready for review.

There was an existing issue with the /bulkThreads api, the threads were not returned correctly sorted desc/asc order date wise, fixed here, not sure if it was for any kind of optimization?

mzparacha commented 1 year ago

@masvelio Thanks for the review.

Was trying to view but the video doesn't load here

Update: It was resolved

mzparacha commented 1 year ago

Just noticed the popover going RTL on small screens (its intended to go RTL on big screens).

@meeshlin confirming if we should make the popover go LTR on small screens when the filters stack like this?

image
mzparacha commented 1 year ago

  1. It looks like pagination does not work properly for other sort than Newest. In Newest I can see bunch of threads loaded but when I select likes or comments, only 2-3 next pages are loaded. There are issues with oldest as well

    Kapture.2023-05-22.at.12.18.38.mp4

  2. Previously we had a tick next to the selected option. Now we do not have any indicator which row is selected? Is this on purpose? Maybe worth to ask about that the design team. image image

Nice find. I did some digging and this is an incompatibility of our new filters with our current implementation of the cuttoff_date.

Will be updating the /bulkThreads api to our standard pagination approach

meeshlin commented 1 year ago

Feedback:

Redlines

Redlines

Resources:

@mzparacha @jnaviask

masvelio commented 1 year ago

@mzparacha I just saw the comment from @meeshlin and I started wondering if we should use mui base for the select component instead of writing it from the scratch. The functionality and styling looks very similar as expected behaviour/style and correct me if I am wrong but I thing that the design team will base their work on mui-base components?

meeshlin commented 1 year ago

Happy to work with MUI base, there will be a lot of overlap with components @masvelio

cc: @zakhap @jnaviask @MuonShot

mzparacha commented 1 year ago

@meeshlin Thanks for the feedback. Will be fixing those details soon.

I just saw the comment from @meeshlin and I started wondering if we should use mui base for the select component instead of writing it from the scratch

@masvelio nice catch. The new select component is actually based on already existing logic that we had in stages_menu.tsx, i shifted it over to its more configurable container, under the hood its applying the same logic as it did before.

About Mui, not sure if we have a standard approach for markup? i do see instances where we are using mui but again not sure on what's the preferred fallback for when we dont. Unrelated but I would actually argue to either use mui or not to do so entirely, its just more opinionated third-party to maintain and since its related to the how our app looks, maybe we should go for more control like some standardization -- ex: should we always incline towards third party even or if the component/markup is easy to build in-house we go for it? A date library like moment would be a mess to make in-house but component library tailored to the exact design needs no so maybe. -- just some random thoughts, maybe it doesn't apply in our case but wanted to leave it out here..

Update:

correct me if I am wrong but I thing that the design team will base their work on mui-base components

not sure about this, maybe @meeshlin can confirm?

just saw this reply above, maybe i was typing this msg and missed it.

zakhap commented 1 year ago

Happy to work with MUI base, there will be a lot of overlap with components @masvelio

cc: @zakhap @jnaviask @MuonShot

100% we should use MUI Base for all custom components we add to the Component Kit! Though if it is quicker to fix the logic in the bug in the video below than it is to rewrite the components using MUI Base, then we should ship this PR when the bug is fixed and then update the component later.

Either:

  1. Fix bug in current logic, Ship PR
  2. Rewrite component, QA again for bugs, the ship PR.

https://github.com/hicommonwealth/commonwealth/assets/16248081/902ad353-1676-48a6-9302-6d10a8bd409e

meeshlin commented 1 year ago

Just noticed the popover going RTL on small screens (its intended to go RTL on big screens).

@meeshlin confirming if we should make the popover go LTR on small screens when the filters stack like this? image

Yes, whichever way works best without it going outside of the frame size.

mzparacha commented 1 year ago

This is applied in 096f3dc

mzparacha commented 1 year ago

@meeshlin Addressed this in https://github.com/hicommonwealth/commonwealth/pull/3926/commits/c737b6d604a47b8906735ffdaad6029bc057509f

Looks like this when the filter row is full width

image

and this when the row stacks

image
jnaviask commented 12 months ago

cc @zakhap @meeshlin is this PR ready to go? Still deployed on demo.

zakhap commented 12 months ago

@jnaviask @mzparacha, this passes product usage CA.

Now waiting on @meeshlin or @jessmart1213 to Design QA.

jnaviask commented 12 months ago

@meeshlin test

meeshlin commented 12 months ago

Looks great! Just a few pieces of feedback before shipping (note: I'm making some improvements on the design system side to the states, but no need to have them implemented to ship the feature at the moment)

  1. The type in the dropdown button field is Body 02/Regular (Neue Haas Unica, size: 16px / 1rem, line-height: 24px / 1.5rem, spacing: 1%) and the color is Neutrals/800 (#282729)

  2. For any dropdown menus that have a scroll, keep it exposed all the time so the user knows that they can scroll and there's more items in the list at first glance

    That's all from me.

jnaviask commented 12 months ago

cc @mzparacha to fix merge conflicts and address feedback, then this PR is g2g

mzparacha commented 12 months ago

The type in the dropdown button field is Body 02/Regular (Neue Haas Unica, size: 16px / 1rem, line-height: 24px / 1.5rem, spacing: 1%) and the color is Neutrals/800 (#282729)

@meeshlin i tried but it looks a little bigger

Image image

For now, i commented the font-size, line-height and font-weight here which looks like this

Image image
mzparacha commented 12 months ago

We merged the thread state pr recently which uses thread data already in store if there is any instead of making an api call. This pr got created before the thread pr got merged, and in this pr we added filter actions which filters thread based on applied filters. Since we also read from store so some changes are needed for filtering data from state here.

I believe we could make a followup ticket, but would be better to handle it in this pr IMO.

@CowMuon @jnaviask If you agree then will make some changes tomorrow first thing to handles these cases.

jnaviask commented 12 months ago

@mzparacha feel free to make the suggested changes. We'll get this merged tomorrow.

meeshlin commented 12 months ago

Bigger is good because we want to make sure that accessibility-wise, everything is easily readable.

Sort and filter copy

Dropdown button

cc: @jessmart1213

mzparacha commented 12 months ago

@meeshlin Ready for review!

mzparacha commented 12 months ago

Bigger is good because we want to make sure that accessibility-wise, everything is easily readable.

@meeshlin - just had a thought (not related to this PR/ticket) - I got the gist about accessibility for color but Are we aiming for accessibility for other areas as well, like keyboard navigation, control elements (things like forms) accessibility etc?

mzparacha commented 12 months ago

@jnaviask Just for clarity, do we want to refetch threads when the featured filter (newest, oldest etc) changes? -- if not the user might see some incorrectly ordered data based on whats available in state...

meeshlin commented 12 months ago

@mzparacha moving forward, definitely need to keep accessibility top-of-mind beyond colors. Hence why we're adding focus states and increasing font sizes across the board in the design system, but these are tiny pieces of the puzzle. While it's out-of-scope for feature work that's ready for implementation, we definitely want to be doing all we can on the design side to ensure we're passing WCAG standards.

I know there's also work to be done on the FE side...tabbing order, and so forth...but it's up to you all to figure out the best approach.

In any case, everything looks great, though we did make a minor change which was to reduce the border of the container to 1px on the inside (box-sizing: border-box;). Right now because the border is on the outside, it's increasing the component size and we want to make sure it remains at 40px (or whichever multiple of 8).

mzparacha commented 12 months ago

@jnaviask fixed this

@meeshlin Thanks for the suggestions, updated the border, now the height is 40px.