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

Add paper-chips & paper-contact-chips #527

Merged
merged 57 commits into from
Nov 22, 2016

Conversation

pauln
Copy link
Contributor

@pauln pauln commented Oct 27, 2016

Continued from PR #501 (itself continued from PR #344). Now that the paper-menu branch has been merged, this has been rebased on latest master; some additional tweaks have also been applied to make the autocomplete dropdown behaviour within chips more consistent with AM.

mansona and others added 30 commits October 28, 2016 11:26
…ectly

Angular Material's styles for contact chips expect the chips to be within an md-chips tag inside the md-contact-chips tag.
The "remove" buttons in chips were default paper-icon size (24), which is too big to fit nicely in the chips.  18 seems about right.
Bottom border is now visible and changes colour depending on focus.
Allows for keyboard navigation of existing chips, including removal.  This doesn't (yet) extend to contact chips.
Make contact chips keyboard navigable by extending chips.
This allows for regular chips to use autocomplete, optionally allowing the user to add their own values in addition to those offered by autocomplete.
@pauln
Copy link
Contributor Author

pauln commented Nov 3, 2016

Readonly fix merged into this PR - thanks @ibarrick!

@miguelcobain
Copy link
Collaborator

@ibarrick i don't know why, but the last example "Contact Chips" is still focusable after readonly is true (I see the line turning orange underneath).

@pauln
Copy link
Contributor Author

pauln commented Nov 3, 2016

Sorry, I should've realised that the fix needed to go on contact chips too (as they have separate templates due to visual and behavioural differences). Thanks again @ibarrick, and well spotted @miguelcobain!

@miguelcobain
Copy link
Collaborator

@pauln @ibarrick
Here is the result of my review.

1

The contact-chips dropdown is too small to display the contact's e-mail. It is hidden on the right side.

screen shot 2016-11-03 at 22 44 32

2

virtual repeat seems to trigger unnecessary rerenders. In this example, we're using an image from a web service that basically delivers random people images. The fact that a new image appears tells me that the item gets rerendered. The AM demo also uses this service and this flickering doesn't happen. This may be related with virtual repeat and not with chips?

rerender issue

3

I think contact-chips are not flexible enough. We're using hardcoded property paths for the contact details: {{item.image}}, {{item.name}} and {{item.email}}.
Instead, we could use the get helper like {{get item imageField}}, {{get item nameField}} and {{get item emailField}}. We should define the default field paths in the component's js.

@pauln
Copy link
Contributor Author

pauln commented Nov 4, 2016

1

We've got a couple of options here, as I see it: we can either drop the email address onto a second line (the image should give sufficient height to get two lines of text in comfortably), or make the dropdown wider.

I tried disabling matchTriggerWidth on the autocomplete, but it seemed to result in the dropdown disappearing completely - I think we'll need to apply some custom styles (possibly using dropdownClass to apply a custom class, depending on how easily we can target it already) if we want to make it wider.

Please feel free to either suggest an approach or implement one which works and submit a PR ;-)

2

I just threw a lorempixel image into a paper-autocomplete custom item template, and can confirm that the re-render happens there too. The issue is therefore either with virtual repeat or paper-autocomplete - fixing it there should fix it here too.

3

Great idea - I've just made name, email, image and search fields configurable and added in a demo.

@ibarrick
Copy link
Contributor

ibarrick commented Nov 7, 2016

@miguelcobain I'm able to reproduce the issue in chips and vanilla autocomplete, but not with vanilla virtual-repeat. Do you have any idea what sort of thing would cause this?

@miguelcobain
Copy link
Collaborator

@ibarrick could it be something related with mutation observers?

@ibarrick
Copy link
Contributor

ibarrick commented Nov 7, 2016

@miguelcobain That was one of the things I looked for this morning. I did end up fixing it finally though and submitted a PR to @pauln. The issue ended up being the fact that the visibleItems array computes an array of new POJO's everytime it recalculates. This causes ember to think that all the items have changed since the references changed, even though the values had not, and so ember re-renders the whole list.

I was able to fix it by keeping a parallel array of the pojos that would have been generated by the visibleItems CP and just returning a sub-array from that.

fixed virtual repeat for autocomplete
@pauln
Copy link
Contributor Author

pauln commented Nov 7, 2016

Thanks, @ibarrick! That leaves us with the email address visibility issue; @miguelcobain, do you have any thoughts on either imposing a minimum width on the dropdown (which still mightn't fix the issue if anyone in the list has a particularly long name) or dropping the email address onto a second line? The second option differs from AM slightly, but I think it's an improvement - and it'd actually make it match the list of contacts which is visible below the contact-chips in the AM demo. At a glance, I'd expect either option to need some of the AM styles to be overridden; I'd appreciate some guidance on doing so, as I haven't done that within ember-paper yet.

@miguelcobain
Copy link
Collaborator

miguelcobain commented Nov 7, 2016

I've detected a new bug, and I think it is a bit problematic. :/

Things to notice:

  1. two items are highlighted after a scroll
  2. the selected item is not the one I clicked in (the gif didn't record the cursor, but I clicked "Anita Gros")

nov 07 2016 21 24

@pauln
Copy link
Contributor Author

pauln commented Nov 7, 2016

Whoops - that seems to be a regression caused by @ibarrick's fix for the VR issue. I've reverted it for now.

Revert "fixed virtual repeat for autocomplete"
@ibarrick
Copy link
Contributor

ibarrick commented Nov 7, 2016

Where's the fun in fixing bugs if you don't create more?

@ibarrick
Copy link
Contributor

@miguelcobain I removed the CPM's and switched to using the key attribute of each as you suggested. Everything appears to be working well now, once @pauln merges we should be good to go :)

@pauln
Copy link
Contributor Author

pauln commented Nov 18, 2016

Going back to the contact-chips dropdown width issue, I've worked out what I need to do to make it appear at a suitable width (350px seems about right). What's the best way to add in a single CSS rule that applies to contact chips? I've slipped it into app/styles/ember-paper.scss for testing, and it works fine there; is it ok to add it there, or should I create a new stylesheet (and if so, where?) just for this one rule?

@miguelcobain miguelcobain merged commit 0a0becf into adopted-ember-addons:master Nov 22, 2016
@miguelcobain
Copy link
Collaborator

Thanks to the champions @pauln and @mansona. 🏆

@mansona
Copy link
Collaborator

mansona commented Nov 24, 2016

Wow this is a great day 🎉 Thanks everyone and especially @pauln for taking the baton and bringing it home 👍

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.

4 participants