search

hicommonwealth / commonwealth

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

Adds new Design System SearchBarcomponent #4462

Closed martins87 closed 8 months ago

martins87 commented 10 months ago

Link to Issue

Closes: #4119

Description of Changes

jnaviask commented 10 months ago

@martins87 please fix linter errors

jnaviask commented 10 months ago

@martins87 please reply to Marcin's comments noting whether the raised issue has been addressed, and if not then why

martins87 commented 10 months ago

@martins87 please reply to Marcin's comments noting whether the raised issue has been addressed, and if not then why

Done.

masvelio commented 10 months ago
  1. when I click inside the input but very close to the border, the focus is not being activated

https://github.com/hicommonwealth/commonwealth/assets/14819225/6ad91d2c-4f54-44f7-bab2-b74f974df3a9

  1. active border (light blue) is activated only when I start typing. Shouldn't it be activated on input focus? Also when I type something (or select the item) and then click outside, the active state does not go away

https://github.com/hicommonwealth/commonwealth/assets/14819225/ba3aafd6-4c5f-463d-bfa5-907769914383

  1. probably it would be nice if in this state I hit backspace, the selected item is removed

image

  1. It is quite common to use keyboard arrows to operate this type of search (up & down) and enter to select the item. Currently it is not supported.

  2. We don't have any indicator if the typed value has not march -- something like no options in the dropdown

All of these above are just my thoughts that are usually implemented in this kind of component. You can play around with the searchbox from mui to have a sense of the good UX here. I am not saying though we need to implement all of these in this PR. This is a decision for @zakhap & @meeshlin.

As I said earlier, those kind of components are really hard to write from scratch, because there are many edge cases, let alone accessibility.

zakhap commented 10 months ago

Of the issues @masvelio brought up above, we should resolve 1, 2, + 5.

  1. We don't have any indicator if the typed value has not march -- something like no options in the dropdown

"No results found" should be the copy.

Q: Does the new CWTag interfere with the tag would @Miaplacidus was doing?

jnaviask commented 10 months ago

@martins87 please fix typecheck issue

martins87 commented 10 months ago

  1. I'm not getting this behaviour when clicking close to the border. Currently when hovered inside the component, the border turns blue. Testing it on localhost, because storybook was broken when ChainInfo was added. Will check this again later on.

  2. As per Figma designs, blue shadow border should be activated only when typing. State is now off when clicked away.

  3. Addressed.

  4. Yet to be implemented.

  5. Addressed.

Cc @masvelio @zakhap

martins87 commented 10 months ago

Of the issues @masvelio brought up above, we should resolve 1, 2, + 5.

  1. We don't have any indicator if the typed value has not march -- something like no options in the dropdown

"No results found" should be the copy.

Q: Does the new CWTag interfere with the tag would @Miaplacidus was doing?

Please check number list comments above and let me know your thoughts.

I don't think the CWTag interfere on the one you mentioned because it receives the chain as props and displays chain name and icon.

masvelio commented 10 months ago

@martins87

  1. As per Figma designs, blue shadow border should be activated only when typing. State is now off when clicked away.

No, there is no state for typing as I pointed out in the other comment. It should have border on hover and border+shadow on focus. Thats it

  1. Yet to be implemented.

Zak confirmed that you do not need to implement it

martins87 commented 9 months ago

@martins87

  1. As per Figma designs, blue shadow border should be activated only when typing. State is now off when clicked away.

No, there is no state for typing as I pointed out in the other comment. It should have border on hover and border+shadow on focus. Thats it

  1. Yet to be implemented.

Zak confirmed that you do not need to implement it

  1. Indeed. It has been addressed.

  2. Ok.

martins87 commented 9 months ago

Note: the component is working fine on /components. Its story on storybook although is broken because of some error with ChainInfo importing. I'll dig into this.

zakhap commented 9 months ago

Behavior for search bar:

CowMuon commented 9 months ago

Late to the party here, but the very first thing I'm noticing is that the PR description here isn't even filled out correctly. @martins87 what happened?

martins87 commented 9 months ago

Late to the party here, but the very first thing I'm noticing is that the PR description here isn't even filled out correctly. @martins87 what happened?

Done.

martins87 commented 9 months ago

Behavior for search bar:

  • If in community, automatically show the community tag for filtering search results.

    • Tag can be x’d out, which removes the filter from the search query/results.

    • The tag cannot be recreated by the user.

    • the search field is only for the search query, not for deciding/inputing filters (like by community).

  • If not in a community, the community tag should not be present
  • If the tag is present, do not search for or display communities. All other content should be shown: comments, threads, etc.
  • If the tag is not present, include communities + all other items in the dropdown

All addressed.

martins87 commented 9 months ago

please see the comments for code changes. Also would be good to push it to the demo right away and let Jess know to test it out

Done. I've tagged Jessica to review it.

martins87 commented 9 months ago

codewise looks good, approving 👍

Things TODO before merge:

  • [x] resolve conflicts
  • [ ] get design signoff
  • [x] get rid of drag&drop commits/code from this PR

Cc @jessmart1213

jessmart1213 commented 9 months ago

Design QA feedback

Hi @martins87 I just reviewed updates and everything looks great! Thank you!

I do have an additional piece of feedback that I did not catch in the first Design QA round, which was that the updated search bar has a fixed width at all breakpoints, when it should be responsive. This is important because a search bar with a fixed width blocks user functionality at smaller breakpoints, as the search pushes the utility nav (notification cta, profile dropdown, etc) out of view.

Expected responsive behavior is what we have currently implemented. Please see attached screen recording for more context.

https://github.com/hicommonwealth/commonwealth/assets/133259695/8e794e8c-a18a-4160-aedc-2b9966ce976c

I'm not sure if the search bar being responsive was part of the acceptance criteria for this ticket, so lmk if this requires a separate ticket!

martins87 commented 8 months ago

Design QA feedback

Hi @martins87 I just reviewed updates and everything looks great! Thank you!

I do have an additional piece of feedback that I did not catch in the first Design QA round, which was that the updated search bar has a fixed width at all breakpoints, when it should be responsive. This is important because a search bar with a fixed width blocks user functionality at smaller breakpoints, as the search pushes the utility nav (notification cta, profile dropdown, etc) out of view.

Expected responsive behavior is what we have currently implemented. Please see attached screen recording for more context.

Screen.Recording.2023-08-29.at.6.50.51.PM.mov I'm not sure if the search bar being responsive was part of the acceptance criteria for this ticket, so lmk if this requires a separate ticket!

Addressed. Deployed to demo for testing.

Cc @jessmart1213 @jnaviask

masvelio commented 8 months ago

@jnaviask @martins87 @jessmart1213 lets make sure that this will be merged before we start working on Gating. This component will be needed in Groups Display Page

jessmart1213 commented 8 months ago

The search bar component has Design sign-off! All my requested updates have been met. Looks great @martins87! cc @masvelio

martins87 commented 8 months ago

The search bar component has Design sign-off! All my requested updates have been met. Looks great @martins87! cc @masvelio

@jnaviask