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] Create Custom Multi-Select Dropdown and Style Seasonal Planting Guide #62

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

kylezryr
Copy link
Collaborator

@kylezryr kylezryr commented Dec 7, 2024

What's new in this PR 🧑‍🌾

Description

  • Refactored FilterDropdownMultiple and FilterDropdownSingle to use Select component from react-select
  • Applied styling changes to dropdowns
  • Applied styling changes to /seasonal-planting-guide
  • Created SeasonalColorKey in seasonal planting guide and PlantCardKey in view plants

Screenshots

image
image
image
image
image

How to review

  • Go to /seasonal-planting-guide and check that the styling matches
  • Use the filters and check that styling matches, check that filters still work to filter out rows

Next steps

  • Styling for clear filters button
  • Changing styling of multi select dropdowns depending on designs

Relevant links

Online sources

https://react-select.com/styles
https://react-select.com/components

Related PRs

CC: @ccatherinetan

@ccatherinetan ccatherinetan linked an issue Dec 7, 2024 that may be closed by this pull request
package.json Outdated Show resolved Hide resolved
@kylezryr kylezryr force-pushed the 60-create-custom-multi-select-dropdown branch from c1ad295 to 30a6d4f Compare December 7, 2024 23:38
Copy link
Collaborator

@ccatherinetan ccatherinetan left a comment

Choose a reason for hiding this comment

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

haven't gotten a chance to go over super in depth, but it is looking amazing; since there's a console error, i haven't merged yet.

the remaining steps are

  1. resolve the console error
  2. update the PR description

<input
type="checkbox"
checked={props.isSelected}
style={{ marginRight: 8 }} // spacing between checkbox and text
Copy link
Collaborator

Choose a reason for hiding this comment

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

i'm getting a console error:
"You provided a checked prop to a form field without an onChange handler. This will render a read-only field. If the field should be mutable use defaultChecked. Otherwise, set either onChange or readOnly."

perhaps we need to pass an onChange here?

here's more from the console error
Screen Shot 2024-12-13 at 2 19 48 AM

@kylezryr kylezryr force-pushed the 60-create-custom-multi-select-dropdown branch from 2208fca to a98306c Compare December 13, 2024 19:00
@@ -82,6 +85,9 @@ export default function Page() {
const [searchTerm, setSearchTerm] = useState<string>('');
const [selectedPlants, setSelectedPlants] = useState<Plant[]>([]);
const [ownedPlants, setOwnedPlants] = useState<OwnedPlant[]>([]);
const [isCardKeyOpen, setIsCardKeyOpen] = useState<boolean>(false);
Copy link
Collaborator

@ccatherinetan ccatherinetan Dec 23, 2024

Choose a reason for hiding this comment

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

optimization note: each time isCardKeyOpen changes, the entire page will re-render, which is expensive and unnecessary, so we should consider

  1. maybe separate out the PlantCardList out into a separate component (and wrap it in a memo), or
  2. wrap the PlantCardList section (i.e. everything under the title) in a useMemo? -- this would do the same thing but would mitigate having to define a new component

for more on re-rendering / how to prevent it: https://www.developerway.com/posts/react-re-renders-guide

however, we've already wrapped PlantCard in a memo, so maybe it's chill? something to consider tho!

@ccatherinetan ccatherinetan force-pushed the 60-create-custom-multi-select-dropdown branch from a2513e8 to 9e06512 Compare December 29, 2024 23:04
Copy link
Collaborator

@ccatherinetan ccatherinetan left a comment

Choose a reason for hiding this comment

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

looking amazing! a couple final things

  • Do we want the hasValue styling (i.e. background-colour becomes shrub when the filter has a value) to apply for the Multi Select as well?
    • (It’s only present for the SingleSelect currently) → would need to modify multiValueLabel, control, dropdownIndicator just as was done for FilterDropdownSingle
  • miscellaneous styling (see my comments)
    • remove the ref for IconButton?
    • cover the drop shadow above View Plants
    • ensure that the filter borders in View Plants aren't cut off

bigger changes that we can consider doing in a separate PR

  • Consider combining the multi select and single select into a single FilterDropdown component (see my comment for reasoning)
    • Or, at least reuse the styling between the two (single source of truth!)
  • Consider creating a FilterGroup component to be used in both view plants and planting timeline (see my comment for reasoning)
  • decide how to handle the forwardRef for PlantCardKey

I was able to resolve part of the console error, but the remaining error regards aria, an assistive screen-reader tool, in particular aria-activedescendant

  • More on aria: Proposal: use combobox role JedWatson/react-select#4695
  • Honestly I can’t figure this one out (and it seems other ppl on Stack Overflow also haven’t) without understanding aria more in depth, but lowk, I think we can just leave it 😔 …
  • error message: + aria-activedescendant={undefined} - aria-activedescendant=""

? COLORS.shrub
: '#fff',
padding: '8px 14px',
color: COLORS.midgray,
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it necessary to set the text color here? the placeholder styling is defined below, and we aren't rendering any text outside of the dropdown

(the same comment applies for the FilterDropdownMultiple.)

import styled from 'styled-components';
import COLORS from '@/styles/colors';
import { DropdownOption } from '@/types/schema';

export const FilterDropdownInput = styled.select<{ $hasValue: boolean }>`
Copy link
Collaborator

Choose a reason for hiding this comment

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

FilterDropdownInput isn't being used anywhere, right? do we want to comment this out or otherwise remove it?

hideSelectedOptions={false}
// use custom styled components instead of default components
components={{ MultiValue: StyledMultiValue, Option: CustomOption }}
menuPosition="fixed"
Copy link
Collaborator

@ccatherinetan ccatherinetan Dec 30, 2024

Choose a reason for hiding this comment

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

i was able to fix the id hydration error by adding the line below
instanceId="dropdown-single"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since there are a lot of shared styles between FilterDropdownMultiple and FilterDropdownSingle, we should consider consolidating the styles. A hacky way to do define the shared styles in FilterDropdownMulti/styles.ts and import the shared styles in FilterDropdownMulti.

However, perhaps we can also consider consolidating the FilterDropdownSingle and FilterDropdownMultiple into just FilterDropdown, which takes isMulti as an additional prop.

I remember you said that you separated them b/c it was getting to unwieldy. I do think it makes things slightly more complicated so perhaps that can be in a separate PR.

However, combining these into FilterDropdown would also make it easier to create the FilterGroup component (see comment in view-plants)

Comment on lines +21 to +24
export const customSelectStyles = <T>(
$isSmall: boolean,
): StylesConfig<DropdownOption<T>, true> => ({
// container
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was gonna say that customSelectStyles can just be a constant rather than a function, but I see that you need to pass in the $isSmall prop.

If we want to keep this as a constant rather than a function, we can define this constant inside the FilterDropdownSingle component so that it directly has access to all the states.

But honestly maybe it's fine as is

Copy link
Collaborator

@ccatherinetan ccatherinetan Dec 30, 2024

Choose a reason for hiding this comment

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

is the only reason we have the $isSmall b/c we want the Select State dropdown to be wide enough to fit the state options without horizontally scrolling?

Currently, I made the menu options wide enough on it's own (max-content, rather than inheriting the parent dropdown's width); would this mitigate the need for this "small" prop?

we should consult with @kyrenetam

View Plants
</H1>
<Flex $direction="row" $gap="10px" $align="center">
<H1 $color={COLORS.shrub} $fontWeight={500}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can we get the TopRowContainer to cover the dropshadow from the Header?
Screen Shot 2024-12-29 at 10 38 55 PM

Comment on lines +426 to +428
<div ref={cardKeyRef}>
<PlantCardKey />
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: instead of wrapping PlantCardKey in other div, it might be cleaner to use forwardRef, which would look smth like this:

export const PlantCardKey = forwardRef<HTMLDivElement>((props, ref) => {...} 
PlantCardKey.displayName = 'PlantCardKey';

HOWEVER, apparently, in React 19 (we're using React 18.3) you can directly pass ref as a prop, so maybe we can hold off on this change.
Apparently React 19 might introduce some breaking changes, so upgrading might be done in a separate PR

</div>
)}
</div>
</Flex>
<SearchBar searchTerm={searchTerm} setSearchTerm={setSearchTerm} />
<FilterContainer>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Screen Shot 2024-12-29 at 10 50 24 PM

In View Plants, the upper/lower borders of the filters are being cutoff. This issue is already addressed in the Planting Timeline page by adding 1px padding to top and bottom. I think this is another argument for consolidating the filters in View Plants and Planting Timeline into a cobined component: FilterGroup which can take in props: CompleteFilter[], where CompleteFilter is typed

filterProps: FilterDropdownProps<T> // coming from FilterDropdownMultiple
checkFilterMatch: (plant: Plant) => boolean,

Then we could refactor filteredPlantList like so

// filterFuncs = completeFilters.map(cf => cf.checkFilterMatch);
const filteredPlantList = useMemo(() => 
  plants.filter(plant =>
      filterFuncs.every(fn => fn(plant));
), [filterFuncs];

this can be done in a separate PR, but I think consolidating the filters into a single component would be cleaner, since it creates a single source of truth and keep styling consistent

padding: '0px',
margin: '0px',
fontSize: '0.75rem',
color: state.hasValue ? `#fff` : `${COLORS.black} !important`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

note: my understanding is that singleValue will only be rendered if a value is selected, so this check is kinda redundant (i.e. state.hasValue will always be true, so we can just directly assign the text color to be white)

Comment on lines +67 to +69
border: '0px',
padding: '0px',
margin: '0px',
Copy link
Collaborator

Choose a reason for hiding this comment

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

note: we can also directly use the unstyled prop in Select, which would remove all non-essential styles, mitigating the need to set these to 0. however, if that interferes with other styling, this is ok

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.

Create Custom Multi-Select Dropdown
2 participants