-
Notifications
You must be signed in to change notification settings - Fork 173
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
Simplify form #1446
Simplify form #1446
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1446 +/- ##
=============================================
- Coverage 67.16% 34.06% -33.10%
- Complexity 15 99 +84
=============================================
Files 5 14 +9
Lines 67 273 +206
=============================================
+ Hits 45 93 +48
- Misses 22 180 +158
Continue to review full report at Codecov.
|
I need help for the last part :)
|
80e11eb
to
2b213c6
Compare
Ready to review now! :) If someone wants to help with the "displaying placeholders for social profiles, see also #1445" you are welcome, otherwise I think we can also finish this in a follow-up. |
Screenshots? |
<span class="property__value property__title--right"> | ||
<div>{{ readableName }}</div> | ||
<!-- display tooltip with hint if available --> | ||
<div v-if="info" v-tooltip.auto="info" class="property__title--icon-details icon-details" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still disagree with this.
We had to add this because lots of users are confused and vCards is complicated.
I don't want to remove it and I think it doesn't create any issues to leave it there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you ever see something like this in another popular Contacts app?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's ask the question differently, what would you say to a user that says they doesn't understand the difference between RELATED & RELATIONSHIP ? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To that I would say that I fixed the wording via 715df61
→ to 'Relationship to you' and 'Related contacts'
;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it more clear? Doesn't really seems to me 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it’s quite clear. One thing is to put in the "Relationship to you" andthe other thing is to put in "Related contacts". Not sure how much clearer it can be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, your call!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.property__title--icon-details { |
to cleanup as well
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
715df61
to
d2d1062
Compare
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Fixed your feedback @skjnldsv and rebased as well, ready to review @nextcloud/contacts. :) |
@skjnldsv not sure about the waiting reports, they don’t seem to come back? Want to merge? :) |
That should trigger it again :) |
{ id: 'FACEBOOK', name: 'Facebook', placeholder: 'https://facebook.com/…' }, | ||
{ id: 'GITHUB', name: 'GitHub', placeholder: 'https://github.com/…' }, | ||
{ id: 'GOOGLEPLUS', name: 'Google+', placeholder: 'https://plus.google.com/…' }, | ||
{ id: 'INSTAGRAM', name: 'Instagram', placeholder: 'https://instagram.com/…' }, | ||
{ id: 'LINKEDIN', name: 'LinkedIn', placeholder: 'https://linkedin.com/…' }, | ||
{ id: 'PINTEREST', name: 'Pinterest', placeholder: 'https://pinterest.com/…' }, | ||
{ id: 'QZONE', name: 'QZone', placeholder: 'https://qzone.com/…' }, | ||
{ id: 'TUMBLR', name: 'Tumblr', placeholder: 'https://tumblr.com/…' }, | ||
{ id: 'TWITTER', name: 'Twitter', placeholder: 'https://twitter.com/…' }, | ||
{ id: 'WECHAT', name: 'WeChat', placeholder: 'https://wechat.com/…' }, | ||
{ id: 'YOUTUBE', name: 'YouTube', placeholder: 'https://youtube.com/…' }, | ||
{ id: 'MASTODON', name: 'Mastodon', placeholder: 'https://mastodon.social/…' }, | ||
{ id: 'DIASPORA', name: 'Diaspora', placeholder: 'https://joindiaspora.com/…' }, | ||
{ id: 'OTHER', name: 'Other social media', placeholder: 'https://example.com/…' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep @call-me-matt, agree with both your points. :) I'd say in the interest of small steps, we should merge this and do those in separate pull requests though? Especially as only showing the username instead of full URL will need some backend stuff, and ppl might have put in urls with https or http, with or without "www.", with or without "mobile." (in the case of Twitter). |
I think that is not too difficult to implement. For the avatar download I have already done that for most use cases. Usually its only the basename-function that is required :) |
Need help on displaying placeholders for social profiles, see also #1445