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

feat: add search pill to filters #3763

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

CzarekDryl
Copy link
Contributor

Description

  • Added search pill next to filters
  • When clicking 'enter' key while typing in search input - filter menu closes
image

Testing

  • Go to any page with filters i.e. activity feed, balances
  • Open filters menu and use search
  • Click 'enter' key and check if menu closes

Resolves #3666

@CzarekDryl CzarekDryl self-assigned this Nov 25, 2024
@CzarekDryl CzarekDryl requested a review from a team as a code owner November 25, 2024 08:28
@mmioana mmioana self-requested a review November 26, 2024 15:32
Copy link
Contributor

@mmioana mmioana left a comment

Choose a reason for hiding this comment

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

Nice start on this issue @CzarekDryl 🙌

Tested your changes and it looks good on desktop
Screenshot 2024-11-26 at 16 33 55
Screenshot 2024-11-26 at 16 34 04
Screenshot 2024-11-26 at 16 34 26
Screenshot 2024-11-26 at 16 34 32
Screenshot 2024-11-26 at 16 35 34
Screenshot 2024-11-26 at 16 35 45
Screenshot 2024-11-26 at 16 37 01

However, on mobile, I did notice the modal has a scroll even though there is enough space for it not to be shown

Screen.Recording.2024-11-26.at.17.02.49.mov

Also I left some refactoring comments which should help keep our codebase more maintainable, if you can please tackle them 🙏

Comment on lines 23 to 33
const [isSearchOpened, setIsSearchOpened] = useState(false);
const isMobile = useMobile();
const { getTooltipProps, setTooltipRef, setTriggerRef, visible } =
usePopperTooltip({
delayShow: 200,
delayHide: 200,
placement: 'bottom-start',
trigger: 'click',
interactive: true,
});
const { getTooltipProps, setTooltipRef, setTriggerRef } = usePopperTooltip({
delayShow: 200,
delayHide: 200,
placement: 'bottom-start',
trigger: 'click',
interactive: true,
visible: isSearchOpened,
onVisibleChange: setIsSearchOpened,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Given this code

const [isSearchOpened, setIsSearchOpened] = useState(false);

const { getTooltipProps, setTooltipRef, setTriggerRef } = usePopperTooltip({
    delayShow: 200,
    delayHide: 200,
    placement: 'bottom-start',
    trigger: 'click',
    interactive: true,
    visible: isSearchOpened,
    onVisibleChange: setIsSearchOpened,
  });

is duplicated in 6 files could we please extract it into a global hook useSearchFilter

const useSearchFilter = () => {
  const [isSearchOpened, setIsSearchOpened] = useState(false);
  const { getTooltipProps, setTooltipRef, setTriggerRef } = usePopperTooltip({
    delayShow: 200,
    delayHide: 200,
    placement: 'bottom-start',
    trigger: 'click',
    interactive: true,
    visible: isSearchOpened,
    onVisibleChange: setIsSearchOpened,
  });

  const closeSearch = () => {
    setIsSearchOpened(false);
  }

  const openSearch = () => {
    setIsSearchOpened(true);
  }

  const toggleSearch = () => {
    setIsSearchOpened((prev) => !prev);
  }

  return {
    getTooltipProps,
    setTooltipRef,
    setTriggerRef,
    isSearchOpened,
    closeSearch,
    openSearch,
    toggleSearch
  }
};

and then use it here?

  const {
    isSearchOpened,
    openSearch,
    closeSearch,
    toggleSearch,
    getTooltipProps,
    setTooltipRef,
    setTriggerRef,
  } = useSearchFilter();

Comment on lines 27 to 36
const [isSearchOpened, setIsSearchOpened] = useState(false);
const { getTooltipProps, setTooltipRef, setTriggerRef } = usePopperTooltip({
delayShow: 200,
delayHide: 200,
placement: 'bottom-end',
trigger: 'click',
interactive: true,
visible: isSearchOpened,
onVisibleChange: setIsSearchOpened,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the useSearchFilter hook here

Comment on lines 24 to 34
const [isSearchOpened, setIsSearchOpened] = useState(false);
const isMobile = useMobile();
const { getTooltipProps, setTooltipRef, setTriggerRef, visible } =
usePopperTooltip({
delayShow: 200,
delayHide: 200,
placement: 'bottom-start',
trigger: 'click',
interactive: true,
});
const { getTooltipProps, setTooltipRef, setTriggerRef } = usePopperTooltip({
delayShow: 200,
delayHide: 200,
placement: 'bottom-start',
trigger: 'click',
interactive: true,
visible: isSearchOpened,
onVisibleChange: setIsSearchOpened,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the useSearchFilter hook here

Comment on lines 24 to 34
const [isSearchOpened, setIsSearchOpened] = useState(false);
const isMobile = useMobile();
const { getTooltipProps, setTooltipRef, setTriggerRef, visible } =
usePopperTooltip({
delayShow: 200,
delayHide: 200,
placement: 'bottom-end',
trigger: 'click',
interactive: true,
});
const { getTooltipProps, setTooltipRef, setTriggerRef } = usePopperTooltip({
delayShow: 200,
delayHide: 200,
placement: 'bottom-end',
trigger: 'click',
interactive: true,
visible: isSearchOpened,
onVisibleChange: setIsSearchOpened,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the useSearchFilter hook here

Comment on lines 32 to 41
const [isSearchOpened, setIsSearchOpened] = useState(false);
const { getTooltipProps, setTooltipRef, setTriggerRef } = usePopperTooltip({
delayShow: 200,
delayHide: 200,
placement: 'bottom-end',
trigger: 'click',
interactive: true,
visible: isSearchOpened,
onVisibleChange: setIsSearchOpened,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the useSearchFilter hook here


return (
<>
<div className="flex flex-row gap-2">
{!!searchValue && !isMobile && searchPill}
Copy link
Contributor

Choose a reason for hiding this comment

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

And then use the new component here

        {!isMobile && (
          <SearchPill value={searchValue} onClick={() => onSearch('')} />
        )}

@@ -93,6 +103,7 @@ function Filter<TValue extends FilterValue>({
<MagnifyingGlass size={14} />
</Button>
)}
{!!searchValue && isMobile && searchPill}
Copy link
Contributor

Choose a reason for hiding this comment

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

And then use the new component here

        {isMobile && (
          <SearchPill value={searchValue} onClick={() => onSearch('')} />
        )}

@@ -66,23 +76,27 @@ const TeamsPageFilter: FC<TeamsPageFilterProps> = ({
return (
<>
<div className="flex flex-row gap-2">
{!!searchValue && !isMobile && searchPill}
Copy link
Contributor

Choose a reason for hiding this comment

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

And then use the new component here

        {!isMobile && (
          <SearchPill value={searchValue} onClick={() => onSearch('')} />
        )}

>
<MagnifyingGlass size={14} />
</Button>
{!!searchValue && searchPill}
Copy link
Contributor

Choose a reason for hiding this comment

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

And then use the new component here

        <SearchPill value={searchValue} onClick={() => onSearch('')} />

@@ -64,6 +74,7 @@ const Filter: FC<FilterProps> = ({
>
<MagnifyingGlass size={14} />
</Button>
{!!searchValue && searchPill}
Copy link
Contributor

Choose a reason for hiding this comment

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

Also use the new component here

         <SearchPill
            value={searchValue}
            onClick={() => setSearchValue('')}
          />

@CzarekDryl
Copy link
Contributor Author

@mmioana Thanks for comments, I think scroll in search modal should be fixed in other issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add filter active search pill
2 participants