feat(search drawer: content accordion): implemented content accordion with dummy data

Description

Added accordion component to search drawer to display content data. Data is dummy due to missing data in redux store which is going to be introduced in next PR

Things done

• Moved search drawer content to separate component
• Proposed dynamic loaded content to drawers in drawer.component
• refactored drawer redux -> idea is to open drawers by triggering action "open" + "drawerName"
• added jest-watch-typeahead to allow search by filename during test watch
• added helper function getReduxWrapperUsingSliceReducer to allow creation of redux store for given reducer and use it for test purposes with wrapped component

Things not done

• Decided to skip tests for search drawer content component. I'm going to introduce changes in PR where I connect data to API

requested review from @AdrianOrlow and @mateuszmiko

We exported the literal types and named them PathName. Ofc, we should also rename the file with the types.

Weird conditional. Why having class translate-x-0 defines if the drawer is opened?

If we change drawer style a bit and translate-x-0 won't be used anymore, this test will fail.

IMO we should use some other conditional, for eg. data- attribute or class named opened

Let's imagine scenario: You create css class

.open { @apply translate-x-0 (...other properties here) }

and in test you check for existence of the class (it might be also attribute, it's the same for this example). Now... someone works with the component and styles, and removes @apply translate-x-0 . The test will pass but code is broken - it's false positive test. It's because you test code instead of behaviour.

Translate-x-0 is now main class that is responsible for showing drawer. I know it's not perfect and it would be better to mount / dismount entire component

someone works with the component and styles, and removes @apply translate-x-0 . The test will pass but code is broken - it's false positive test. It's because you test code instead of behaviour.

That's exactly the case also right now. That is why I proposed using opened css class name - both in the component and its test, for eg.:

expect(screen.getByRole('drawer')).not.toHaveClass('opened');

In the case above false positive result of the test is not possible, only in case if someone removes opened class but that's is probably impossible.

There are some issues:

1. If we add more conditionals like this, it will look very unclean

{open && drawerName === 'search' && <SearchDrawerContent />}
{open && drawerName === 'other' && <OtherDrawerContent />}
{open && drawerName === 'random' && <RandomDrawerContent />}

So I propose to exclude this logic to function and memoize it with use of useMemo to prevent unnecessary rerenders

2. IMO it's a good idea to make drawerName a constant. If we will ever want to change some drawer content component name, it will be much easier to do this with use of a constant than a string

Ad1. Ok I moved it to function:

const isSearchDrawerOpened = (): boolean => isOpen && name === 'search' 

I'm not going to wrap ip in useMemo. There is no benefit of doing it.

ad 2. I think is outdated comment now since I explained redux usage here?

@teoMiesiac isSearchDrawerOpened is not a solution for this problem.

There will be more possible drawer contents and we should not create a new function for each one of them

That's why I proposed excluding whole logic (of getting final content component) to other function and memoize it. For eg:

const getDrawerContent = (drawerName: DrawerType): React.FC => {
  const contentConfig: Record<DrawerType, React.FC> = {
    search: SearchDrawerContent,
    other: SearchDrawerContent,
    random: RandomDrawerContent
  }

  return contentConfig[drawerName]
}

const Content = useMemo(() => getDrawerContent(drawerName), [drawerName])

...

return (
  ...
  {open && <Content/>}
  ...
)

Ofc this is only an example

The second comment is not outdated IMO because it applies to readability of the code only - constant would only replace magic strings but won't change logic. In the example above:

const contentConfig: Record<DrawerType, React.FC> = {
    [DrawerName.Search]: SearchDrawerContent,
    [DrawerName.Other]: SearchDrawerContent,
    [DrawerName.Random]: RandomDrawerContent
  }

Overall LGTM but IMO Redux is an overkill here

approved this merge request

mentioned in commit 66410e7a

merged