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

added basic select field #1391

Closed
wants to merge 5 commits into from
Closed

added basic select field #1391

wants to merge 5 commits into from

Conversation

langan
Copy link

@langan langan commented Aug 14, 2015

Added basic select field for #854

@tleunen
Copy link
Contributor

tleunen commented Aug 14, 2015

Could you add a screenshot? I'm interested to see the result!

@langan
Copy link
Author

langan commented Aug 14, 2015

closed - http://grab.by/JCsy
open - http://grab.by/JCsI

Would be happy to make an element page if needed

@langan
Copy link
Author

langan commented Aug 14, 2015

The markup to add this looks like this

            <div class="mdl-selectfield mdl-js-selectfield" tabindex="-1">
                <select name="orderby" class="mdl-selectfield__select">
                    <option value="date">Date</option>
                    <option value="price_asc">Price Asc</option>
                    <option value="price_desc">Price Desc</option>
                    <option value="popularity" selected="selected">Popularity</option>
                </select>
            </div>

@surma
Copy link
Contributor

surma commented Aug 14, 2015

Thanks for the PR, @langan. Haven’t looked at it just yet, but we do appreciate the effort!

For completeness sake (and for the occasional lazy colleague), here’s a link to the relevant spec.

@@ -48,3 +48,4 @@
@import "tooltip/tooltip";
@import "shadow/shadow";
@import "grid/grid";
@import "selectfield/selectfield";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be alphabetically placed in the list. Shadow and Grid should always be last.

@Garbee
Copy link
Collaborator

Garbee commented Aug 14, 2015

How is this going to look on mobile?

I'm a bit uneasy about the implementation. It appears like it will hide the main select input and then create a duplicate menu on-the-fly to display. This can hinder form accessibility which we should not do at all costs.

@langan
Copy link
Author

langan commented Aug 14, 2015

The menu updates the original select element when it is changed.

@Garbee
Copy link
Collaborator

Garbee commented Aug 14, 2015

Well, the underlying select menu has to be updated. I don't however believe that means the accessibility defaults an option provides continues to exist. We will need to do testing to verify there are not any adverse a11y problems. If there are, then they need to be addressed before including something like this.

@tleunen
Copy link
Contributor

tleunen commented Aug 14, 2015

Re-using the Menu is really great. I was thinking the same thing.
Having the menu on top of the default select is the best we can do to have a real custom styles for this.

But I'm not sure about using a button to display the selected value. Seems like over re-using what has been written so far, but maybe we want a button so user knows they can click (and for a11y that's fine too).
What will be the underline animation? Should we have one? The menu will be over it anyway...

@langan
Copy link
Author

langan commented Aug 15, 2015

@tleunen At the moment you see the underline animation just before the menu opens. I used the button because I don't think we can achieve the MD style in the spec using just CSS on the select and make it cross browser compatible.

@Garbee
Copy link
Collaborator

Garbee commented Aug 15, 2015

I don't think we can achieve the MD style in the spec using just CSS on the select and make it cross browser compatible.

We need to give that a shot first. It would provide the full a11y capabilities of an option node without any extra work. Only when we have a list of exact issues with handling the component that way that can't be resolved or are too difficult to resolve should we fallback to this method (and then improve it.)

@chrispantazis
Copy link

@Garbee select boxes cannot be CSS'ed to look like the specs of the MD guidelines. Have a look here:
https://css-tricks.com/dropdown-default-styling/

The only cross browser implementation is to replace the original selects (which by default are styled by the OS) with some other elements.

The "--full-width modifier" is so that we can make the select form field behave like all the other input elements (we've added it since its also on a modifier on the textfields on MDL).

Eventually most of the CSS in this select box will have to just extend (with SASS) the text field functionallity.

@Garbee
Copy link
Collaborator

Garbee commented Aug 17, 2015

select boxes cannot be CSS'ed to look like the specs of the MD guidelines. Have a look here:
https://css-tricks.com/dropdown-default-styling/

Can you please point out the actual issues? Just checking the font not working, it does in Chrome/Blink currently. So that shows the other data here could be outdated.

(we've added it since its also on a modifier on the textfields on MDL). (About --full-width.)

Just because something is in the current items doesn't mean it needs to be brought into new items. The modifiers for width shouldn't exist and will be removed in the next major. Thinking about this, we shouldn't make upgrading more strenuous and add extra weight to the library just to have a small layer of consistency.

Width modifiers were a mistake in design. They should have been removed when the grid was implemented but we failed to do that. It doesn't mean we should continue adding them in as they are unnecessary.

@chrispantazis
Copy link

@Garbee The element is considered an "ugly" widget because it's impossible to style it consistently cross platform. Both the select and the popup options are controlled by the OS and not the browser. However, some things are possible. Changing the bg color, border etc (basic styling). No of these options though are enough to style it as defined by the MD specs.

@Garbee
Copy link
Collaborator

Garbee commented Aug 18, 2015

I know the difficulties of styling select elements. However, if we can get an exact list of issues in implementing Material Design with native elements perhaps we can take that to the spec team and work something out.

Doing this method loses mobile-native select UX which is critical to good usability. Especially in large option menus. I'm simply asking for due diligence being done for any additions. With forms it is even more critical that any implementations be thoroughly cared for. We can work with the MD team to try and address issues native to the web instead of just saying the current spec is the absolute proper method in all cases.

This PR as it is may end up doing more harm to sites than good in the long run. Much more work needs to be put into it (and the menu component) before this could be considered. As it is, this is just a complete hack.

@addyosmani
Copy link
Contributor

I've reviewed the approaches taken by three different teams at Google attempting to add MD select components to their libraries.

The common outcome to date has been that if you're going to rely on the native select element, you concede to shipping a piece of UI that looks a little Material, but doesn't fully follow the spec. All implementations to date that have been able to match the spec have (unfortunately) had to go down the route of implementing a custom select instead.

Examples of areas of difficulty include option highlight/hover colours not matching as the OS and browser/theme may impact this. For MDL, we'll need to make a call on whether we would prefer to ship a lightweight MD select component that is mostly okay and relies on native or something that follows the spec entirely.

The MD team otherwise leave it up to the end-developer to make a call on whether they're prepared to take on board the additional accessibility work needed to ship the full-fidelity component.

@Dunexus
Copy link

Dunexus commented Aug 21, 2015

Few months ago, I tried to do something looking like material design select (https://www.google.com/design/spec/components/menus.html#menus-behavior) by using radio and label. It works on desktop and on touchscreens.

Here is a JSFiddle demo : https://jsfiddle.net/jvkgLr01/

@yantakus
Copy link

yantakus commented Sep 2, 2015

So, any updates on this? Select field is a component used in almost every app.

@addyosmani
Copy link
Contributor

No updates yet :) Given the complexity of getting this component right we still need to make a call on whether to target this for the next large release or one after it. The best option to keep track is to subscribe to notifications on this issue. That way once we've got movement you'll get pinged.

@langan
Copy link
Author

langan commented Sep 2, 2015

I've updated the branch with a few fixes from the above comments & closing the select which we needed for our project

  • select now grabs the innerHTML not the value of the currently selected option
  • removed --full-width modifier
  • the select now closes when you click anywhere on the page

@brunoksato
Copy link

+1

@Garbee
Copy link
Collaborator

Garbee commented Sep 18, 2015

Please do not +1 things. That only causes noise for people watching the issues. If you want to comment, add something constructive to the discussion.

@langan
Copy link
Author

langan commented Sep 23, 2015

@addyosmani In case you want to see the pull request code in action it is being used on our latest template here. @Garbee I made the country select have lots of options in order to test it on mobile, it scrolls pretty well for me.

We made some slight tweaks in our template's version of the select, would be happy to update the pull request with them if you think it's worth continuing.

@addyosmani addyosmani added this to the V2 milestone Oct 1, 2015
@ghost
Copy link

ghost commented Oct 12, 2015

Any updates to this PR? I'm currently implementing a web app with mdl-lite and would love to have first-class select field support.

@Garbee
Copy link
Collaborator

Garbee commented Oct 12, 2015

It is going to take some time before select is supported by MDL. There is a lot of testing that needs to be done before we can tell where this patch stands compared to what we need to provide. We will update this PR with anything relevant to it and the upstream issue with anything relevant to the more general implementation as we have something to provide.

@Garbee Garbee removed this from the V2 milestone Dec 24, 2015
@Garbee
Copy link
Collaborator

Garbee commented Dec 24, 2015

I'm clearing the milestone on this since we may not have the ability to make sure it gets in with 2.0.

Adding in a custom select alternative is quite intensive to make sure it not only fully aligns to Material Design as much as possible but also is accessible. Which means possibly giving into native on mobile for the most accessible experience.

We know having selects is very important. But we have major architectural rewrites coming in 2.0 along with reworking existing components heavily. We can attempt to work on selects as we have available time. It is unlikely we will for 2.0.

We simply can't integrate a component that we know beforehand will cause severely inaccessible forms. That isn't good for us or developers using the component.

@Garbee
Copy link
Collaborator

Garbee commented Jan 10, 2016

I'm actually going ahead and closing this PR entirely. The codebase is having a major architectural change in 2.x. Along with how we think about our components in the first place. Nothing from this PR can be salvaged in the redesign.

Subscribe to the upstream issue #854 for more details as they become available.

Thanks.

@Garbee Garbee closed this Jan 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.