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

[SelectField] Port component #6301

Closed
wants to merge 9 commits into from
Closed

Conversation

kybarg
Copy link
Contributor

@kybarg kybarg commented Mar 8, 2017

This is PR is just to start discussion on #5716 and #4792 as for now

Desired functionality:

Overall progress

  • Component
  • Tests (at least unit tests)
  • Docs
  • Demo

@muibot muibot added PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Mar 8, 2017
>
{React.Children.map(children, (child, index) =>
React.cloneElement(child, {
selected: value === child.props.value,
Copy link
Contributor

@avocadowastaken avocadowastaken Mar 8, 2017

Choose a reason for hiding this comment

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

I also suggest add custom comparator that by default will compare strictly

Copy link
Contributor

Choose a reason for hiding this comment

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

and use any prop type for value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@umidbekkarimov You want to have ability to set custom comparison operator like == or ===, etc?

Copy link
Contributor Author

@kybarg kybarg Mar 8, 2017

Choose a reason for hiding this comment

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

@muibot

and use any prop type for value

I want to stick with native html <select> element.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mostly deepEqual, I use { id, name } objects across my project alot, and pass them as a value prop to SelectField.

Copy link
Contributor

Choose a reason for hiding this comment

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

It worth discussion, cause previous API supported this http://www.material-ui.com/#/components/select-field

And it would be another breaking change.

Anyways it's easy to write wrapper over it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@umidbekkarimov You are right, fixed both 😄

@mbrookes mbrookes added the next label Mar 8, 2017
@oliviertassinari
Copy link
Member

@nathanmarks Might have already started migrating the select component. What's the status on that?

@oliviertassinari oliviertassinari removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Mar 11, 2017
@kybarg
Copy link
Contributor Author

kybarg commented Mar 15, 2017

@oliviertassinari any updates?

@@ -0,0 +1,10 @@
# Select Filed

Choose a reason for hiding this comment

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

Typo: Filed -> Field

@oliviertassinari
Copy link
Member

@kybarg I'm hoping that @nathanmarks could bring us some light 💡 . He made quite some progress with the component but he lost his work by mistake.
I will look at it tomorrow otherwise.

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 26, 2017

In case that might help, I have been cleaning the issues for the SelectField component. Here is the list That's a challenging component to port to next!

I have been trying the component out locally, here is the first issue I noticed:

bubu

@muibot muibot added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI PR: Needs Review and removed PR: Needs Review labels Apr 10, 2017
@dummerbd
Copy link

Is anyone actively working on this? I've been hacking on the work done here and I'm wondering if using an actual <select> element is the best way to go. The work done here works very well for mouse interaction but there's possibly an issue with styling on OSX (the non-focused underline looks weird?) and the select element interferes with proper keyboard nav since it opens up when tabbed to instead of the menu opening.

I've been looking at the way Angular-Material (original and v2) build their select component and they're using "divs" in a similar way to MUI's master branch impl. From what I can tell, using a <select> makes reusing the Input component much easier, is there any other reason to use it? Might it be easier to use divs instead?

@dummerbd
Copy link

Oh, I see in the first post that we want the <select> for autofill to work.

@dummerbd dummerbd mentioned this pull request Apr 14, 2017
7 tasks
@kybarg
Copy link
Contributor Author

kybarg commented Apr 14, 2017

@dummerbd Sorry, switched to work on TextField lately)
And yes I want to wrap native select element not to loose browser features. I don't have Safari to test but I will dig on that) @oliviertassinari also reported that issue

@oliviertassinari oliviertassinari added on hold There is a blocker, we need to wait and removed PR: Needs Review labels Apr 15, 2017
@oliviertassinari oliviertassinari removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Apr 15, 2017
@oliviertassinari
Copy link
Member

I'm adding the on hold state to this PR as @dummerbd is continuing the effort 😄 .

@oliviertassinari oliviertassinari added the component: select This is the name of the generic UI component, not the React module! label Apr 15, 2017
@oliviertassinari
Copy link
Member

I don't think that we need to keep this PR opened. I'm closing.

@nathanmarks nathanmarks mentioned this pull request Jun 7, 2017
4 tasks
@kybarg kybarg deleted the next-select-field branch August 1, 2017 15:01
@kybarg kybarg mentioned this pull request Aug 18, 2017
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: select This is the name of the generic UI component, not the React module! on hold There is a blocker, we need to wait
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants