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

show email for attendee #108

Merged
merged 3 commits into from
Nov 22, 2016
Merged

show email for attendee #108

merged 3 commits into from
Nov 22, 2016

Conversation

tcitworld
Copy link
Member

@tcitworld tcitworld commented Oct 1, 2016

I would like to have some … at the end when they're truncated, but uib-typeahead doesn't seem to accept ternaries directly, so I guess we need a full filter just for this.

Fixes #45

@tcitworld tcitworld added the 3. to review Waiting for reviews label Oct 1, 2016
@mention-bot
Copy link

@tcitworld, thanks for your PR! By analyzing the annotation information on this pull request, we identified @georgehrke, @raghunayyar and @jancborchardt to be potential reviewers

@coveralls
Copy link

coveralls commented Oct 1, 2016

Coverage Status

Coverage remained the same at 29.07% when pulling 28be30d on show-email-attendee into c95f0eb on master.

@raghunayyar
Copy link
Member

👍

@jancborchardt
Copy link
Member

@tcitworld is it maybe possible to ellipsize just using CSS and going with the width? Normal ellipsizing is achieved using:

white-space: nowrap;
overflow: hidden;
text-overflow: ellipsis;

@tcitworld
Copy link
Member Author

I'll try.

@tcitworld
Copy link
Member Author

Still the same issue on mobile, but at least it's okay on desktop view.

@coveralls
Copy link

coveralls commented Oct 4, 2016

Coverage Status

Coverage remained the same at 29.07% when pulling 26a7966 on show-email-attendee into c95f0eb on master.

@raghunayyar
Copy link
Member

@nextcloud/designers anyone up for a review? :)

@raghunayyar
Copy link
Member

@georgehrke please review, its long pending. :)

@jancborchardt
Copy link
Member

@raghunayyar can you post a quick screenshot?

@tcitworld
Copy link
Member Author

@jancborchardt
Desktop

Mobile

(Don't mind the display of the button, it's a Firefox Test Pilot issue)

@jancborchardt
Copy link
Member

So as per the requirement in #45:

otherwise a user is not able to distinguish which email address is used.

it’s only shown when a name is duplicate, right? Because normally it’s not necessary to show it. :)

And yeah, it would be good to get the ellipsizing down, should be possible by restricting the width of the input. cc @nextcloud/designers anyone wants to help out here? :)

@georgehrke
Copy link
Member

@tcitworld Can you please only display the email address when there are multiple email addresses available for a user :)

@georgehrke georgehrke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Nov 7, 2016
@ghost
Copy link

ghost commented Nov 22, 2016

@georgehrke @tcitworld What about starting with the basic functionality as provided here and adding visual enhancements later on?

  • showing email addresses only when contact has multiple email entries?
  • maybe just showing the email domain part? (but that would collapse again in specific cases)
  • select behavior via a setting switch in the calendar app?
    Etc.
    As most of my contact entries have multiple email addresses (work and home) it's very hard to figure out which is the correct name when setting up event invitations.
    So thanks @tcitworld as I was searching for that functionality, but my JS/PHP skills are to bad ;-)

@georgehrke
Copy link
Member

maybe just showing the email domain part? (but that would collapse again in specific cases)

Then some emails will only have the domain and others will have the entire email address.
We should try to keep it consistent. Also I don't see a benefit in only showing the domain.

select behavior via a setting switch in the calendar app?

I'm sorry, but this won't happen. We are trying to make things simple. Showing the email address when multiple are available and hiding it when only one is available covers all usecases.
Adding a setting doesn't solve anything imho and only clutters the user interface. :)

@georgehrke georgehrke self-assigned this Nov 22, 2016
@georgehrke
Copy link
Member

Changed the behavior to the expected one:

calendar - nextcloud chromium today at 1 44 03 pm

please review @jancborchardt @tcitworld @eppfel

@georgehrke georgehrke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 22, 2016
@tcitworld
Copy link
Member Author

Oh, I thought I had pushed the change.
LGTM 👍

@georgehrke georgehrke merged commit cba7123 into master Nov 22, 2016
@georgehrke georgehrke deleted the show-email-attendee branch November 22, 2016 15:07
@skjnldsv
Copy link
Member

Are you using the select2 function? This should have the same design as tags, admin//workflow or other selects on nextcloud :)

@georgehrke
Copy link
Member

Not yet.
Is there proper documentation on select2 and presumably select1/select?

@skjnldsv
Copy link
Member

I don't know! 😞
But systemtags is using it. https://select2.github.io

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants