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

FR: Add way to easily access the selected item #5

Open
diondiondion opened this issue Sep 21, 2020 · 7 comments
Open

FR: Add way to easily access the selected item #5

diondiondion opened this issue Sep 21, 2020 · 7 comments
Labels
enhancement New feature or request

Comments

@diondiondion
Copy link
Contributor

Hi, thanks for releasing the latest version of this lib!

I just started implementing a Select component using use-item-list and have run into an issue trying to display the label of the selected item on the button that opens the menu.

The issue is that the Select component should only receive the value of the selected item, to mirror the native <select> element's API. However, getting the selected item's text from itemList proved harder than I thought, as itemList.items is initially empty, and there's no built-in way to access the currently selected item.

For now I'm working around it with some extra state, passing the setter down into context and calling it from an effect within the Option component when selected === true.

// In the parent Select.js
const [selectedItem, setSelectedItem] = useState();

// In the child Option.js
useEffect(() => {
  if (selected) {
    setSelectedItem({
        ref: itemRef,
        text: children,
        value,
      });
    }
}, [selected, children, value]);

While this works fine for now, it seems like use-item-list should come with a built-in hook (e.g. useSelectedItem) that returns the selected item once it's "ready".

@souporserious souporserious added the enhancement New feature or request label Sep 21, 2020
@souporserious
Copy link
Owner

I agree, there should be a first-class way to do this. I'll work on this when I get a chance. Feel free to start a PR if you have a solution in mind in the meantime!

@diondiondion
Copy link
Contributor Author

diondiondion commented Sep 23, 2020

My current workaround has the significant downside of the selected item not being available on the first render, so in a SSR context the Select component wouldn't show the name of the selected option.

This can also be seen here https://5app.github.io/base5-ui/src-select-readme, where the menu actually stays blank until you interact with it. This doesn't happen in our non-SSR'd app, but for some reason on Docz the item list doesn't seem to get populated properly, which means the selected property from useItem isn't updated & the effect isn't run.
Never mind, that was just a silly mistake in the documentation's code example. Sorry 🤦

@diondiondion
Copy link
Contributor Author

Feel free to start a PR if you have a solution in mind in the meantime!

I've started working on this on the weekend, hope to be able to submit a PR later this week.

My proposed solution would be quite similar to my user-land workaround. Unless I'm missing something, I don't think there's a way to make the selected item's data available in the parent on the first render.

It's a bit of a shame, but I suppose it's a limitation of React's component model. And it doesn't have to be a deal-breaker, as component implementations that need to access the selected item on first render for SSR could always implement some sort of getInitiallySelectedItemLabel function to provide a fallback (or use a React.Children-based approach, but that would kind of defeat the point of using use-item-list in the first place).

@diondiondion
Copy link
Contributor Author

diondiondion commented Mar 4, 2021

Thanks for the recent PR merge, @souporserious!

I agree, there should be a first-class way to do this. I'll work on this when I get a chance. Feel free to start a PR if you have a solution in mind in the meantime!

I was going to submit a PR for this and #7 yesterday, but didn't manage to wrap it up last night, and now there are merge conflicts as I also changed/cleaned up some of the demos a bit. 😅 But not a big deal. Here's the diff if you want to check it out:

main...diondiondion:track-selected-item

It took me several attempts at more complex approaches that wouldn't force a parent re-render before I ended up where I started: this simple implementation that's very close to the user-land solution.

And it feels a bit like I'm coming to a similar conclusion to you in #7maybe this should be kept outside of the library's scope. 🤔

The other approaches I tried (creating a useSelectedItem hook similar to useHighlightedItemId) all caused very weird side-effects (irregularities in the created items array) that I didn't fully understand the cause of. The hook-within-a-hook pattern & ref mutation during render can be quite hard to wrap your head around. 😅

How attached are you to the hook-returning-a-hook API, btw? It seems like the library could be made much more "canonically Reacty" and more maintainable if the public API was made slightly less elegant:

import {useItemlist, useItem} from 'use-item-list'

function Parent() {}
  const itemList = useItemList({...})
  return (
    <Context.Provider value={itemList}>
      ...
    </Context.Provider>
  )
}

function Child() {}
  const itemList = useContext(Context)
  const item = useItem(itemList, {...})
  return (
    ...
  )
}

The main reason why I feel this tradeoff is acceptable is that the majority of use-cases I have worked with so far required me to pass the useItem hook down via context anyway. So passing down the itemList object directly to then pass it into the useItem hook isn't much more complicated.

@souporserious
Copy link
Owner

Nice! Sorry about the merge conflicts, I was trying to think about how to tackle this last night. Do you think changing to your proposed API will help with getting selected items? It does seem about the same setup and probably not worth maintaining hooks that create hooks if we don't need to.

@souporserious
Copy link
Owner

One thought 🤔 probably against the grain of React, but maybe we can read in render and hydrate selected from there somehow for SSR. If I remember correctly, I think I tried this in the past with no luck on a stable solution.

@diondiondion
Copy link
Contributor Author

Sorry about the merge conflicts, I was trying to think about how to tackle this last night.

No worries, really. :) The conflicts were just in the demos/examples. I'll check out your changes soon, and if there's anything worth adding from my changes I'll make a separate PR for them. Best to keep those separate from features/fixes anyway.

Do you think changing to your proposed API will help with getting selected items?

Not really, no. The changed API would just (hopefully) make the package easier to break up into separate modules and reason about.

One thought 🤔 probably against the grain of React, but maybe we can read in render and hydrate selected from there somehow for SSR. If I remember correctly, I think I tried this in the past with no luck on a stable solution.

Hmm, making this work with SSR sounds really hard. Reading from React.Children.props works on the first render, but then we're back at a less flexible API that doesn't permit component composition or suspended items. What should definitely work is getting the selected item after first render, as soon as all the items are collected, so maybe even in the first useLayoutEffect before things are painted to the screen? That would allow us to at least prevent the flickering before the selected item becomes available (if it's visible in the UI).

I was also randomly reminded today that setting state during render can be fine in certain circumstances (link). Might be another option to look into.

Apart from that we can always add an optional initialSelectedItem option, parallel to initialHighlightedIndex, which could be used if necessary, and point out the caveats around SSR (and probably things like virtualised rendering) in the readme.

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

No branches or pull requests

2 participants