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

Ihor/ray core/rich dropdown component #252

Merged
merged 9 commits into from
Nov 22, 2019

Conversation

sick-sad-world
Copy link
Contributor

@sick-sad-world sick-sad-world commented Nov 13, 2019

Dropdown component for ray framework. When default is not enough. Works as wrapper-enchancer for component. Fallback as .ray-select if JS is not applied for some reason.
https://deploy-preview-252--ray-docs.netlify.com/

@sick-sad-world sick-sad-world requested a review from a team as a code owner November 13, 2019 14:41
@i8ramin
Copy link
Contributor

i8ramin commented Nov 14, 2019

Looks like the icon is overlapping the drop down arrow

5FC35C1C-A782-4AB3-80F9-881641D12ED7

Copy link
Contributor

@adamraider adamraider left a comment

Choose a reason for hiding this comment

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

Pls update your PR title to use conventional commits: https://www.conventionalcommits.org/en/v1.0.0/#summary

@@ -0,0 +1,23 @@
<div className="ray-dropdown">
Copy link
Contributor

Choose a reason for hiding this comment

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

These .html files need to be valid html. These look like JSX, make sure you convert things like className to class, and htmlFor to for

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks. copy-paste plays bad role

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Pokemon
</label>
</div>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure all HTML is valid (not JSX) and please fix the indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

transition-timing-function: ease-in;
transition-property: height, max-height;
}
&-list {
Copy link
Contributor

Choose a reason for hiding this comment

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

delete unused selector

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

There's still Coverage decreased (-4.5%) , could you add more tests? You can view the coveralls report to see which lines need coverage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added more tests

Copy link
Contributor

@adamraider adamraider left a comment

Choose a reason for hiding this comment

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

Looks good, but no keyboard navigation support?


| Method | Params | Description |
| -------------------------- | -------------------- | ------------------------------------ |
| `Select.createAll` | `HTMLElement:Object` | create all instances in document |
Copy link
Contributor

Choose a reason for hiding this comment

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

should be Dropdown.* here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks good, but no keyboard navigation support?

Yeah we split it out on primary and secondary features. Keyboard nav is secondary. We wan't to merge it to ray to unblock and finish GROW-9590. Btw custom language dropdown at footer of wework.com site which was used as prototype also doesn't have keyboard nav

@adamraider adamraider merged commit 59b89b5 into master Nov 22, 2019
@delete-merged-branch delete-merged-branch bot deleted the ihor/ray-core/rich-dropdown-component branch November 22, 2019 21:48
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.

3 participants