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

Merged
merged 31 commits into from
Jan 15, 2025

Conversation

kylezryr
Copy link
Collaborator

@kylezryr kylezryr commented Dec 7, 2024

What's new in this PR 🧑‍🌾

Description

  • Refactored Filter Dropdown Buttons (FilterDropdownMultiple and FilterDropdownSingle)
    • Used and styled React Select
    • Updated filtering logic for checkGrowingSeason to only consider Outdoors Season
  • Seasonal Planting Guide
    • Styled Clear Filters button
    • Styled Filter Dropdowns
    • Re-styled PlantCalendarList (fix scrolling behavior / excessive whitespace)
  • View Plants
    • Added PlantCardKey
  • Miscellaneous changes
    • Deleted Button.tsx file (none of the button styles in that file were being used)
      • However, this file can serve as an example of how to use forwardRef
    • Header
      • Styled the Login/Sign up links
    • Renamed useTitleCase to toTitleCase
      • Impacts PlantCard, YourPlantDetails

Screenshots

note: these screenshots are old; the Clear Filters button is now styled
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

  • 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
  • try to eventually resolve the aria error

bigger changes that we can consider doing in a separate PR

  • Possibly 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! + allows us to do the hasValue styling for both filters)
  • 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 (see comments)

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

Comment on lines 29 to 30
padding-top: 1px;
padding-bottom: 1px; // ensure filter border isn't cut off
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we can apply this padding directly to FilterDropdownSingle and FilterDropdownMultiple so that we don't have to apply this padding each time we want to use these components

@ccatherinetan ccatherinetan merged commit 3fb2764 into main Jan 15, 2025
2 checks passed
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