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

[WIP] SelectBox #197

Closed
wants to merge 5 commits into from
Closed

[WIP] SelectBox #197

wants to merge 5 commits into from

Conversation

pxd3v
Copy link

@pxd3v pxd3v commented Oct 16, 2020

* Creates select box component
* Implements arrow down and arrow up event treatment
* implements scroll to selected item on render
* implements default stories
* implements scrollbars
@vercel
Copy link

vercel bot commented Oct 16, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/arturbien/react95/m09w3ieh0
✅ Preview: https://react95-git-master.arturbien.vercel.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 16, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit f8f2793:

Sandbox Source
React95 template Configuration

Copy link
Member

@luizbaldi luizbaldi left a comment

Choose a reason for hiding this comment

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

Left a couple of comments but it's looking good so far.

src/SelectBox/SelectBox.js Show resolved Hide resolved
src/SelectBox/SelectBox.js Show resolved Hide resolved
src/SelectBox/SelectBox.js Outdated Show resolved Hide resolved
src/SelectBox/SelectBox.stories.js Outdated Show resolved Hide resolved
src/SelectBox/SelectBox.styles.js Show resolved Hide resolved
@luizbaldi luizbaldi added the hacktoberfest-accepted Hacktoberfest valid pull request label Oct 16, 2020
* destructure "key" from event
* changes options map variable name
@arturbien
Copy link
Member

@pxd3v hey! first of all thanks for your work ♥️ there are couple of issues I've noticed in the component tho:

  1. There's some excess border inside the Cutout

Screenshot 2020-10-18 at 13 25 25

Here's the Cutout for comparison:
Screenshot 2020-10-18 at 13 28 00

  1. When controlling the component with a keyboard, scrolling should behave exactly the same as in Select component. Currently the behaviour is different. When you select second option, it's scrolled to the top of the component so the first option is no longer visible. Also we don't want the 'smooth' scrolling

  2. Component should use the same font other components use

  3. We shouldn't use button elements in the SelectBox. You can use this guide for reference: https://www.w3.org/TR/2017/WD-wai-aria-practices-1.1-20170628/examples/listbox/listbox.html . They're simply using ul and li elements with proper aria-labels for accessibility. That's how we also did it in the Select component

* Fix list border
* The list no more uses buttons as inner component for list items
* fix component to use the theme font
* fix scroll behavior and remove smooth scroll
@pxd3v
Copy link
Author

pxd3v commented Oct 18, 2020

Hi @arturbien! Thank you again for the guidance. I just fixed some issues that you've pointed out, in this commit: 89472e2. About the aria-labels, i will study it in order to apply it so we can improve acessibility! Have a nice sunday.

@arturbien
Copy link
Member

@pxd3v thanks! borders and fonts look good now :)

One more style change that comes to my mind- can you change option height to 31px ? Options in Select dropdown have that height and I think it's the optimal size.

Scrolling now works as expected, but I've noticed that it's not possible to set focus to the SelectBox before clicking any of the options. I think it's some kind of regression because previously when I tested it it worked. Can you check what happens there?

* creates first test for component rendering
@pxd3v
Copy link
Author

pxd3v commented Oct 19, 2020

@arturbien I'm glad you like it!

So, I don't know if i understood the SelectBox focus thing, you want the user to have to double click the SelectBox at first? That way he can firstly focus on the selectBox and then focus an option?

Ah, i just commited a new test and the option size fix :)

@arturbien
Copy link
Member

@pxd3v now it looks waaay better! ☺️ What I mean is right now it's NOT possible to focus SelectBox using TAB. User should be able to control the component using only keyboard. Right now user has to click the SelectBox first to be able to control it via keyboard

@pxd3v
Copy link
Author

pxd3v commented Oct 20, 2020

@arturbien got it! Just fixed this issue.

This week i will try to work on some tests, aria-labels and i will also try to study the "flat" display thing!

@pxd3v
Copy link
Author

pxd3v commented Nov 4, 2020

Hi Guys!
This week I've started working in a new company and because of that I'm not having much free time to work on other projects.. I would like to say that I really enjoyed working on this issue but, for now, I will need to pass it to another developer.

Thank you all so much for the attention this whole time and keep going with this amazing project!

@arturbien
Copy link
Member

@pxd3v no worries. thank you for your work, we will take on from here and finish it.

Also congratulations and good luck with your new job! <3

@pxd3v pxd3v closed this Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Hacktoberfest valid pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants