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

Replace react-select with something else #305

Closed
JasonStoltz opened this issue Jul 2, 2019 · 13 comments
Closed

Replace react-select with something else #305

JasonStoltz opened this issue Jul 2, 2019 · 13 comments

Comments

@JasonStoltz
Copy link
Member

react-select was the only somewhat annoying thing to style over with pure CSS, since it uses inline styling. I’m a bit biased in preferring CSS over CSS-in-JS, but it might be worth considering swapping to another accessible library (e.g. react-responsive-select) that makes our method of overriding styling more coherent.

Additionally, the inline styles cause an issue with CSR.

Relates to #93

@JoseRFelix
Copy link
Contributor

Hey Jason I'll be trying this!

@JoseRFelix
Copy link
Contributor

@JasonStoltz Was checking react-responsive-select and it is not able to change its className. Should we change its default styling or change our CSS?

@cee-chen
Copy link

cee-chen commented Sep 3, 2019

Aw man, that kinda sucks that they don't pass a custom className. If we had to, I'd be fine overriding react-responsive-select's default styling/classes in our CSS files.

Just to throw this out there, one other option (if someone else feels really strongly) could be checking out EUI's Select component? 🤷‍♀

@JasonStoltz
Copy link
Member Author

So what are our big requirements here? I think they are that:

  • Should not use inline styles
  • Should be accessible

I'd be fine with considering the EUI Select as long as it's possible to use it without requiring all of EUI as a dependency.

@yakhinvadim
Copy link
Contributor

yakhinvadim commented Sep 3, 2019

I agree with the accessibility requirement, but why we don't want the select to use inline styles?

the inline styles cause an issue with CSR.

@JasonStoltz What's the issue?
Edit: Oh, nevermind, I found it here #93 (comment)

@yakhinvadim
Copy link
Contributor

Hey @Jfelix61, I see that you created a PR replacing react-select with react-responsive-select. I think changing the underlying UI library is a big change for Search UI, what was your reasoning behind choosing react-responsive-select among other libraries? I want to be sure that we're making the right decision here.

@yakhinvadim
Copy link
Contributor

I think react-responsive-select in the first message was just an example, and we should research other libraries as well.
Another option would be to try to improve the accessibility of react-select by making a PR in their repo.

@JoseRFelix
Copy link
Contributor

Hi @yakhinvadim, I researched other libraries with a screen reader enabled and the one that had the best accessibility was react-select (EUI has good accessibility too, but I can't find if it currently has tree shaking).
Since we are having problems with CSR and one of the best I could find without inline CSS among the others was react-responsive-select, I decided to implement that one, which in my opinion is not as good on accessibility than react-select but better than others. If you or anyone of the team has more suggestions, I will be glad to change it promptly. 😁

@yakhinvadim
Copy link
Contributor

the one that had the best accessibility was react-select

Wow, that's great news!
And it looks like the strict CSP issue is fixed in react-select v3 (We're using v2). Unfortunately, we can't upgrade right now, since that would mean that we'd have to drop support for React <16.8.

But my thinking is that since react-select has the best accessibility out there and strict CSP issue is going to fix itself after we upgrade dependencies, it's probably not worth it to replace the library. I expect that if we replace it, we'd face other issues that are already solved by react-select.

Eager to hear your thoughts.

@JoseRFelix
Copy link
Contributor

That's great to know, I love react-select so I have no problems in keeping it. Then, if it's breaking, it would be better to upgrade for the v2.0 breaking version right?

@yakhinvadim
Copy link
Contributor

Yes, I think so.

@JasonStoltz JasonStoltz added this to the 2.0 milestone Oct 31, 2019
@marcvangend
Copy link

the one that had the best accessibility was react-select

Wow, that's great news!
And it looks like the strict CSP issue is fixed in JedWatson/react-select#3585 (We're using v2). >Unfortunately, we can't upgrade right now, since that would mean that we'd have to drop support for React <16.8.

I believe search-ui has depended on react ^16.8 for quite a while now. Does that mean it is possible to upgrade to the latest version of react-select? Personally I have no problem with react-select and it looks like the plans to replace it are not moving forward. Meanwhile, react-select has introduced some accessibility improvements I would love to use.

@JasonStoltz
Copy link
Member Author

We updated react-select recently, and it sounds like it's a lot more pleasant to work with: #776.

I'm going to close this issue.

@JasonStoltz JasonStoltz closed this as not planned Won't fix, can't repro, duplicate, stale Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants