Consolidate user avatar Vue logic
Creates 3 vue components:
user-avatar-linkuser-avatar-imageuser-avatar-svg
and implements them in several places.
Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/31017
-
Port to EE -
Test each implementation manually
EE Compatible Branch: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/1699
Merge request reports
Activity
mentioned in issue #31017 (closed)
added 2 commits
added 336 commits
-
56aec9c0...d3a788da - 335 commits from branch
master - 73418681 - Consolidate user avatar Vue logic
-
56aec9c0...d3a788da - 335 commits from branch
added 1 commit
- e8cd6892 - Revert changes to diff_notes_avatar temporarily.
added 141 commits
Toggle commit listadded 1 commit
- 1d653448 - User double quotes in user_avatar_link.js template.
added 1 commit
- b09d5956 - Use double quotes in user_avatar_image_spec.js template.
added 1 commit
- 9baef22a - Standardize double quotes and component closing brackets.
added 1 commit
- fcb8e046 - Fix diff_note_avatars import of UserAvatarImage.
assigned to @ClemMakesApps
-
@ClemMakesApps can you review?
mentioned in issue #31408 (moved)
added 1 commit
- 6f101892 - Set default_avatar when src and svg are falsey.
added 2 commits
-
@brycepj Vue code should be reviewed by one of the vue experts
https://docs.gitlab.com/ce/development/fe_guide/index.html#vue-features
-
@brycepj left some comments :)
-
Vue code should be reviewed by one of the vue experts
@filipa is there a list somewhere of who this is? I assumed all frontenders are vue experts?
assigned to @brycepj
added 1 commit
- 071e1c14 - Remove duplicated user avatar link in environment item.
-
@brycepj do we have an ETA for this?
It will be needed in order to conclude this https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10987 :)
-
@filipa yeah, sorry, I've been mostly offline the last two days. I believe there are only a few small changes left, so hopefully it'll be ready this coming Monday.
added 2 commits
-
@filipa I made most of the changes you asked for but didn't finish, so don't review yet.
added 2 commits
added 336 commits
-
b8c120e1...e4160fab - 335 commits from branch
master - bfc20d13 - Consolidate user avatar Vue logic
-
b8c120e1...e4160fab - 335 commits from branch
-
@ClemMakesApps @filipa Okay, all discussions resolved. I think this is ready for a final review.
One other thing:
user-avatar-linkreplaceslink-to-member-avatar, which was only in EE. If this gets merged before the MR widget does (it will), there will be several new conflicts to resolve as part of the MR widget.Would you object to me opening an issue to implement
user-avatar-linkin EE once the MR widget has been merged?Edited by username-removed-408230 I'll defer to @filipa on a decision about that
-
-
@brycepj no sorry needed! The MR I was talking about won't get in for %9.2 so no worries
Regarding your question, I would rather make it equal from the beginning. I am afraid we'll forget it and have 2 different ways of doing it, but I understand this will be painful for @fatihacet.
Let's wait for @fatihacet and leave the decision up to him
-
@brycepj when it's ready to review, please assign back to me
-
@filipa If this isn't needed until 9.3, I think it makes sense to just wait for the MR widget and keep the parity between EE and CE.
added 939 commits
-
7b7aa099...3a983b10 - 935 commits from branch
master - c8add1b7 - Consolidate user avatar Vue logic
- 1c809ead - Pass correct props in diff note avatar.
- 3a2800af - Properly compute styles for image element.
- b071291d - Listen for native click event in diff notes avatar.
Toggle commit list-
7b7aa099...3a983b10 - 935 commits from branch
added 3 commits
-
763be10a...09c2aab4 - 2 commits from branch
master - 91ea9da0 - Consolidate user avatar Vue logic
-
763be10a...09c2aab4 - 2 commits from branch
added 1 commit
- a3b6dc3e - Check image for title attribute instead of link.
added 1 commit
- debf9a76 - Add specs for svg and mixin, get all passing.
added 45 commits
-
dadeef78...09c4d27a - 44 commits from branch
master - 5972a26e - Consolidate user avatar Vue logic
-
dadeef78...09c4d27a - 44 commits from branch
-
Couple of notes:
- Why is the user avatar an SVG? The output rendered is always an
<img /> - Please follow the documentation and use .vue files
- Please follow the documentation on importing methods
- Please follow the documentation on props that are not required, they need a default value
Edited by Filipa Lacerda - Why is the user avatar an SVG? The output rendered is always an
-
@brycepj link_to_member_avatar in app/helpers/projects_helper.rb renders a default avatar if none is available. I think we need to keep that logic in the vue component
mentioned in merge request !11302 (closed)
-
One more TODO:
use .vue filesedit: Done 4cebbf60
Edited by username-removed-408230 added 1 commit
- ae47c446 - Use object shorthand and camelcase in componentn naming.
added 1 commit
- 5ee02502 - Add spec for avatarSizeStylesMapString computed value.
- Resolved by username-removed-408230
@filipa An interesting side effect of
TooltipMixinis that we can't test against thetitleattribute in our specs when it's used. That's because when we initialize the tooltip onmount, bootstrap reads from thetitleattribute, setsdata-original-title, and then sets thetitleattribute to an empty string. Before, we didn't initialize the tooltip until hovering over it, so we could expect that thetitleattribute was still set to the tooltip's text.I found this when adding the mixin broke several specs that test for the
titleattribute of a user avatar.So, somewhat ironically, although the
TooltipMixinallows us to not usedata-original-title, I think it may require us to use it in our specs.As of now, I've changed the specs to test for
data-original-title, and they pass. But we could also just remove the use ofTooltipMixinand return to using thewatcherI had earlier. Or maybe you know a better way? WDYT?Edited by username-removed-408230 - Last reply by username-removed-408230
added 99 commits
-
f3380346...61f811e6 - 98 commits from branch
master - 0189c9df - Consolidate user avatar Vue logic
-
f3380346...61f811e6 - 98 commits from branch
assigned to @filipa
-
@brycepj left some comments!
assigned to @brycepj
added 66 commits
-
2887bdb6...75807f2a - 60 commits from branch
master - 2c1a6c30 - Consolidate user avatar Vue logic
- 30856c5c - Fixup karma specs.
- 82b70a57 - Don't evaluate prop expressions where you don't need to.
- 7ed09650 - Remove tooltip class computed.
- 2eab5bcb - Rename href prop to link-href.
- 9c8c64b8 - Assume this.size is always truthy in mixin.
Toggle commit list-
2887bdb6...75807f2a - 60 commits from branch
mentioned in merge request !11340 (merged)
added 1 commit
- 58e4060d - Test that props are included in rendered output.
added 1 commit
- 0af5a4f7 - Fixup user_avatar_image_spec, ensure rendered output is tested.
added 1 commit
-
baf20e04 - Workaround issues using querySelector with
svgelement.
-
baf20e04 - Workaround issues using querySelector with
added 140 commits
-
969ca5b9...afcc81da - 139 commits from branch
master - f6c9a655 - Consolidate user avatar Vue logic
-
969ca5b9...afcc81da - 139 commits from branch
-
@filipa build's passing here. EE branch's build is running, but I've tested the changes manually, so I don't anticipate any failures. I think we're ready to go
-
Thank you @brycepj! Left some comments!
assigned to @filipa
-
@iamphill can you please take over? Thanks!
assigned to @iamphill
assigned to @brycepj
assigned to @iamphill
assigned to @brycepj
added 1 commit
- fa89aa02 - Use DOM methods for testing user_avatar_link.
assigned to @iamphill
assigned to @brycepj
added 158 commits
-
1dce2934...0b946a7b - 156 commits from branch
master - d19dc42e - Consolidate user avatar Vue logic
- 30f4536a - Fixes per feedback from iamphill.
-
1dce2934...0b946a7b - 156 commits from branch
mentioned in issue #32475 (closed)
mentioned in issue #28003 (moved)
added 94 commits
-
688e7a26...a3eabcc2 - 92 commits from branch
master - 3c668fa0 - Consolidate user avatar Vue logic
- b73959a9 - Fixes per feedback on user avatar components.
-
688e7a26...a3eabcc2 - 92 commits from branch
assigned to @iamphill
-
@iamphill Discussions resolved and we're
mentioned in commit b0642299
