Commit 8e85a218 authored by Rajat Jain's avatar Rajat Jain Committed by Mark Florian

Unify image pin style for diffs/design management

The original motivation for this was to make them independent of
the `badge` class, since they weren't really badges.

Part of https://gitlab.com/groups/gitlab-org/-/epics/7174.

Changelog: other
parent 1b2fd94b
<script>
import { mapGetters } from 'vuex';
import imageDiff from '~/diffs/mixins/image_diff';
import DesignNotePin from '~/vue_shared/components/design_management/design_note_pin.vue';
import DraftNote from './draft_note.vue';
export default {
components: {
DraftNote,
DesignNotePin,
},
mixins: [imageDiff],
props: {
......@@ -31,9 +33,12 @@ export default {
class="discussion-notes diff-discussions position-relative"
>
<div class="notes">
<span class="d-block btn-transparent badge badge-pill is-draft js-diff-notes-index">
{{ toggleText(draft, index) }}
</span>
<design-note-pin
:label="toggleText(draft, index)"
is-draft
class="js-diff-notes-index gl-translate-x-n50"
size="sm"
/>
<draft-note :draft="draft" />
</div>
</div>
......
......@@ -694,7 +694,7 @@ export default class Notes {
// Convert returned HTML to a jQuery object so we can modify it further
const $noteEntityEl = $(noteEntity.html);
const $noteAvatar = $noteEntityEl.find('.image-diff-avatar-link');
const $targetNoteBadge = $targetNote.find('.badge');
const $targetNoteBadge = $targetNote.find('.design-note-pin');
$noteAvatar.append($targetNoteBadge);
this.revertNoteEditForm($targetNote);
......
......@@ -286,6 +286,7 @@ export default {
"
:is-inactive="isNoteInactive(note)"
:is-resolved="note.resolved"
is-on-image
@mousedown.stop="onNoteMousedown($event, note)"
@mouseup.stop="onNoteMouseup(note)"
/>
......
<script>
import { GlIcon } from '@gitlab/ui';
import { mapActions } from 'vuex';
import DesignNotePin from '~/vue_shared/components/design_management/design_note_pin.vue';
import noteableDiscussion from '../../notes/components/noteable_discussion.vue';
export default {
components: {
noteableDiscussion,
GlIcon,
DesignNotePin,
},
props: {
discussions: {
......@@ -62,20 +64,22 @@ export default {
<ul :data-discussion-id="discussion.id" class="notes">
<template v-if="shouldCollapseDiscussions">
<button
:class="{
'diff-notes-collapse': discussion.expanded,
'btn-transparent badge badge-pill': !discussion.expanded,
}"
v-if="discussion.expanded"
class="diff-notes-collapse js-diff-notes-toggle"
type="button"
class="js-diff-notes-toggle"
:aria-label="__('Show comments')"
@click="toggleDiscussion({ discussionId: discussion.id })"
>
<gl-icon v-if="discussion.expanded" name="collapse" class="collapse-icon" />
<template v-else>
{{ index + 1 }}
</template>
<gl-icon name="collapse" class="collapse-icon" />
</button>
<design-note-pin
v-else
:label="index + 1"
:is-resolved="discussion.resolved"
size="sm"
class="js-diff-notes-toggle gl-translate-x-n50"
@click="toggleDiscussion({ discussionId: discussion.id })"
/>
</template>
<noteable-discussion
v-show="isExpanded(discussion)"
......@@ -87,9 +91,12 @@ export default {
@noteDeleted="deleteNoteHandler"
>
<template v-if="renderAvatarBadge" #avatar-badge>
<span class="badge badge-pill">
{{ index + 1 }}
</span>
<design-note-pin
:label="index + 1"
class="user-avatar"
:is-resolved="discussion.resolved"
size="sm"
/>
</template>
</noteable-discussion>
</ul>
......
<script>
import { GlIcon } from '@gitlab/ui';
import { isArray } from 'lodash';
import { mapActions, mapGetters } from 'vuex';
import imageDiffMixin from 'ee_else_ce/diffs/mixins/image_diff';
import DesignNotePin from '~/vue_shared/components/design_management/design_note_pin.vue';
function calcPercent(pos, renderedSize) {
return (100 * pos) / renderedSize;
......@@ -11,7 +11,7 @@ function calcPercent(pos, renderedSize) {
export default {
name: 'ImageDiffOverlay',
components: {
GlIcon,
DesignNotePin,
},
mixins: [imageDiffMixin],
props: {
......@@ -36,7 +36,7 @@ export default {
badgeClass: {
type: String,
required: false,
default: 'badge badge-pill',
default: '',
},
shouldToggleDiscussion: {
type: Boolean,
......@@ -114,30 +114,28 @@ export default {
>
<span class="sr-only"> {{ __('Add image comment') }} </span>
</button>
<button
<design-note-pin
v-for="(discussion, index) in allDiscussions"
:key="discussion.id"
:style="getPosition(discussion)"
:class="[badgeClass, { 'is-draft': discussion.isDraft }]"
:disabled="!shouldToggleDiscussion"
class="js-image-badge"
type="button"
:label="showCommentIcon ? null : toggleText(discussion, index)"
:position="getPosition(discussion)"
:aria-label="__('Show comments')"
class="js-image-badge"
:class="badgeClass"
:is-draft="discussion.isDraft"
:is-resolved="discussion.resolved"
is-on-image
:disabled="!shouldToggleDiscussion"
@click="clickedToggle(discussion)"
>
<gl-icon v-if="showCommentIcon" name="image-comment-dark" :size="24" />
<template v-else>
{{ toggleText(discussion, index) }}
</template>
</button>
<button
/>
<design-note-pin
v-if="canComment && currentCommentForm"
:style="{ left: `${currentCommentForm.xPercent}%`, top: `${currentCommentForm.yPercent}%` }"
:aria-label="__('Comment form position')"
class="btn-transparent comment-indicator position-absolute"
type="button"
>
<gl-icon name="image-comment-dark" :size="24" />
</button>
:position="{
left: `${currentCommentForm.xPercent}%`,
top: `${currentCommentForm.yPercent}%`,
}"
/>
</div>
</template>
......@@ -14,7 +14,15 @@ export function createImageBadge(noteId, { x, y }, classNames = []) {
}
export function addImageBadge(containerEl, { coordinate, badgeText, noteId }) {
const buttonEl = createImageBadge(noteId, coordinate, ['badge', 'badge-pill']);
const buttonEl = createImageBadge(noteId, coordinate, [
'gl-display-flex',
'gl-align-items-center',
'gl-justify-content-center',
'gl-font-sm',
'design-note-pin',
'on-image',
'gl-absolute',
]);
buttonEl.textContent = badgeText;
containerEl.appendChild(buttonEl);
......@@ -30,8 +38,8 @@ export function addImageCommentBadge(containerEl, { coordinate, noteId }) {
export function addAvatarBadge(el, event) {
const { noteId, badgeNumber } = event.detail;
// Add badge to new comment
const avatarBadgeEl = el.querySelector(`#${noteId} .badge`);
// Add design pin to new comment
const avatarBadgeEl = el.querySelector(`#${noteId} .design-note-pin`);
avatarBadgeEl.textContent = badgeNumber;
avatarBadgeEl.classList.remove('hidden');
}
......@@ -10,12 +10,12 @@ export function setPositionDataAttribute(el, options) {
}
export function updateDiscussionAvatarBadgeNumber(discussionEl, newBadgeNumber) {
const avatarBadgeEl = discussionEl.querySelector('.image-diff-avatar-link .badge');
const avatarBadgeEl = discussionEl.querySelector('.image-diff-avatar-link .design-note-pin');
avatarBadgeEl.textContent = newBadgeNumber;
}
export function updateDiscussionBadgeNumber(discussionEl, newBadgeNumber) {
const discussionBadgeEl = discussionEl.querySelector('.badge');
const discussionBadgeEl = discussionEl.querySelector('.design-note-pin');
discussionBadgeEl.textContent = newBadgeNumber;
}
......
......@@ -118,7 +118,7 @@ export default class ImageDiff {
removeBadge(event) {
const { badgeNumber } = event.detail;
const indexToRemove = badgeNumber - 1;
const imageBadgeEls = this.imageFrameEl.querySelectorAll('.badge');
const imageBadgeEls = this.imageFrameEl.querySelectorAll('.design-note-pin');
if (this.imageBadges.length !== badgeNumber) {
// Cascade badges count numbers for (avatar badges + image badges)
......
......@@ -61,7 +61,7 @@ export default class ReplacedImageDiff extends ImageDiff {
this.currentView = newView;
// Clear existing badges on new view
const existingBadges = this.imageFrameEl.querySelectorAll('.badge');
const existingBadges = this.imageFrameEl.querySelectorAll('.design-note-pin');
[...existingBadges].map((badge) => badge.remove());
// Remove existing references to old view image badges
......
......@@ -28,12 +28,37 @@ export default {
required: false,
default: false,
},
isOnImage: {
type: Boolean,
required: false,
default: false,
},
isDraft: {
type: Boolean,
required: false,
default: false,
},
size: {
type: String,
required: false,
default: 'md',
validator: (value) => ['sm', 'md'].includes(value),
},
ariaLabel: {
type: String,
required: false,
default: null,
},
},
computed: {
isNewNote() {
return this.label === null;
},
pinLabel() {
if (this.ariaLabel) {
return this.ariaLabel;
}
return this.isNewNote
? __('Comment form position')
: sprintf(__("Comment '%{label}' position"), { label: this.label });
......@@ -51,7 +76,10 @@ export default {
'js-image-badge design-note-pin': !isNewNote,
resolved: isResolved,
inactive: isInactive,
draft: isDraft,
'on-image': isOnImage,
'gl-absolute': position,
small: size === 'sm',
}"
class="gl-display-flex gl-align-items-center gl-justify-content-center gl-font-sm"
type="button"
......
$design-pin-diameter: 28px;
$design-pin-diameter-sm: 24px;
$t-gray-a-16-design-pin: rgba($black, 0.16);
.layout-page.design-detail-layout {
......@@ -12,24 +13,6 @@ $t-gray-a-16-design-pin: rgba($black, 0.16);
top: 35px;
}
.design-note-pin {
display: flex;
height: $design-pin-diameter;
width: $design-pin-diameter;
box-sizing: content-box;
background-color: $purple-500;
color: $white;
font-weight: $gl-font-weight-bold;
border-radius: 50%;
z-index: 1;
padding: 0;
border: 0;
&.resolved {
background-color: $gray-500;
}
}
.comment-indicator {
border-radius: 50%;
}
......@@ -40,35 +23,6 @@ $t-gray-a-16-design-pin: rgba($black, 0.16);
cursor: grabbing;
}
}
/**
* Design pin that overlays the design
*/
.frame .design-note-pin {
box-shadow: 0 2px 4px $t-gray-a-08, 0 0 1px $t-gray-a-24;
border: $white 2px solid;
will-change: transform, box-shadow, opacity;
// NOTE: verbose transition property required for Safari
transition: transform $general-hover-transition-duration linear, box-shadow $general-hover-transition-duration linear, opacity $general-hover-transition-duration linear;
transform-origin: 0 0;
transform: translate(-50%, -50%);
&:hover {
transform: scale(1.2) translate(-50%, -50%);
}
&:active {
box-shadow: 0 0 4px $t-gray-a-16-design-pin, 0 4px 12px $t-gray-a-16-design-pin;
}
&.inactive {
@include gl-opacity-5;
&:hover {
@include gl-opacity-10;
}
}
}
}
.design-scaler-wrapper {
......@@ -177,3 +131,63 @@ $t-gray-a-16-design-pin: rgba($black, 0.16);
.design-card-header {
background: transparent;
}
.design-note-pin {
display: flex;
height: $design-pin-diameter;
width: $design-pin-diameter;
box-sizing: content-box;
background-color: $purple-500;
color: $white;
font-weight: $gl-font-weight-bold;
border-radius: 50%;
z-index: 1;
padding: 0;
border: 0;
&.draft {
background-color: $orange-500;
}
&.resolved {
background-color: $gray-500;
}
&.on-image {
box-shadow: 0 2px 4px $t-gray-a-08, 0 0 1px $t-gray-a-24;
border: $white 2px solid;
will-change: transform, box-shadow, opacity;
// NOTE: verbose transition property required for Safari
transition: transform $general-hover-transition-duration linear, box-shadow $general-hover-transition-duration linear, opacity $general-hover-transition-duration linear;
transform-origin: 0 0;
transform: translate(-50%, -50%);
&:hover {
transform: scale(1.2) translate(-50%, -50%);
}
&:active {
box-shadow: 0 0 4px $t-gray-a-16-design-pin, 0 4px 12px $t-gray-a-16-design-pin;
}
&.inactive {
@include gl-opacity-5;
&:hover {
@include gl-opacity-10;
}
}
}
&.small {
position: absolute;
border: 1px solid $white;
height: $design-pin-diameter-sm;
width: $design-pin-diameter-sm;
}
&.user-avatar {
top: 25px;
right: 8px;
}
}
......@@ -1072,24 +1072,6 @@ table.code {
}
}
.frame .badge.badge-pill,
.image-diff-avatar-link .badge.badge-pill,
.user-avatar-link .badge.badge-pill,
.notes > .badge.badge-pill {
position: absolute;
background-color: $blue-400;
color: $white;
border: $white 1px solid;
min-height: $gl-padding;
padding: 5px 8px;
border-radius: 12px;
&:focus {
outline: none;
}
}
.frame .badge.badge-pill,
.frame .image-comment-badge,
.frame .comment-indicator {
// Center align badges on the frame
......@@ -1121,11 +1103,6 @@ table.code {
}
}
.notes > .badge.badge-pill {
display: none;
left: -13px;
}
.discussion-notes {
min-height: 35px;
......@@ -1134,18 +1111,22 @@ table.code {
min-height: 25px;
}
.diff-notes-expand {
display: none;
}
&.collapsed {
background-color: $white;
.diff-notes-expand {
display: initial;
}
.diff-notes-collapse,
.note,
.discussion-reply-holder {
display: none;
}
.notes > .badge.badge-pill {
display: block;
}
}
}
......
......@@ -9,9 +9,9 @@
-# to the first note position when we click on a badge diff discussion
%ul.notes{ id: "discussion_#{discussion.id}", data: { discussion_id: discussion.id, position: discussion.notes[0].position.to_json } }
- if discussion.try(:on_image?) && show_toggle
%button.gl-button.diff-notes-collapse.js-diff-notes-toggle{ type: 'button' }
%button.comment-indicator.gl-display-flex.gl-align-items-center.gl-justify-content-center.gl-font-sm.diff-notes-collapse.js-diff-notes-toggle{ type: 'button' }
= sprite_icon('collapse', css_class: 'collapse-icon')
%button.gl-button.btn-transparent.badge.badge-pill.js-diff-notes-toggle{ type: 'button' }
%button.gl-align-items-center.gl-justify-content-center.gl-font-sm.small.gl-translate-x-n50.design-note-pin.js-diff-notes-toggle.diff-notes-expand{ type: 'button' }
= badge_counter
= render partial: "shared/notes/note", collection: discussion.notes, as: :note, locals: { badge_counter: badge_counter, show_image_comment_badge: show_image_comment_badge }
......
......@@ -25,7 +25,7 @@
- elsif note_counter == 0
- counter = badge_counter if local_assigns[:badge_counter]
- badge_class = "hidden" if @fresh_discussion || counter.nil?
%span.badge.badge-pill{ class: badge_class }
%span.gl-display-flex.gl-align-items-center.gl-justify-content-center.gl-font-sm.design-note-pin.small.user-avatar{ class: badge_class }
= counter
.timeline-content
.note-header
......
......@@ -56,10 +56,3 @@ button[disabled] {
}
}
}
.frame,
.diff-discussions {
.badge.is-draft {
background-color: $orange-500;
}
}
......@@ -28,7 +28,7 @@ RSpec.describe 'Merge request > User creates image diff notes', :js do
it 'shows indicator and avatar badges, and allows collapsing/expanding the discussion notes' do
indicator = find('.js-image-badge')
badge = find('.image-diff-avatar-link .badge')
badge = find('.image-diff-avatar-link .design-note-pin')
expect(indicator).to have_content('1')
expect(badge).to have_content('1')
......
......@@ -3,6 +3,7 @@ import Vue from 'vue';
import Vuex from 'vuex';
import DiffFileDrafts from '~/batch_comments/components/diff_file_drafts.vue';
import DraftNote from '~/batch_comments/components/draft_note.vue';
import DesignNotePin from '~/vue_shared/components/design_management/design_note_pin.vue';
Vue.use(Vuex);
......@@ -40,10 +41,12 @@ describe('Batch comments diff file drafts component', () => {
it('renders index of draft note', () => {
factory();
expect(vm.findAll('.js-diff-notes-index').length).toEqual(2);
const elements = vm.findAll(DesignNotePin);
expect(vm.findAll('.js-diff-notes-index').at(0).text()).toEqual('1');
expect(elements.length).toEqual(2);
expect(vm.findAll('.js-diff-notes-index').at(1).text()).toEqual('2');
expect(elements.at(0).props('label')).toEqual(1);
expect(elements.at(1).props('label')).toEqual(2);
});
});
......@@ -71,7 +71,7 @@ describe('DiffDiscussions', () => {
expect(diffNotesToggle.text().trim()).toBe('1');
expect(diffNotesToggle.classes()).toEqual(
expect.arrayContaining(['btn-transparent', 'badge', 'badge-pill']),
expect.arrayContaining(['js-diff-notes-toggle', 'gl-translate-x-n50', 'design-note-pin']),
);
});
......@@ -87,8 +87,8 @@ describe('DiffDiscussions', () => {
createComponent({ renderAvatarBadge: true });
const noteableDiscussion = wrapper.find(NoteableDiscussion);
expect(noteableDiscussion.find('.badge-pill').exists()).toBe(true);
expect(noteableDiscussion.find('.badge-pill').text().trim()).toBe('1');
expect(noteableDiscussion.find('.design-note-pin').exists()).toBe(true);
expect(noteableDiscussion.find('.design-note-pin').text().trim()).toBe('1');
});
});
});
import { GlIcon } from '@gitlab/ui';
import { shallowMount } from '@vue/test-utils';
import { mount } from '@vue/test-utils';
import ImageDiffOverlay from '~/diffs/components/image_diff_overlay.vue';
import { createStore } from '~/mr_notes/stores';
import { imageDiffDiscussions } from '../mock_data/diff_discussions';
......@@ -19,7 +19,7 @@ describe('Diffs image diff overlay component', () => {
extendStore(store);
dispatch = jest.spyOn(store, 'dispatch').mockImplementation();
wrapper = shallowMount(ImageDiffOverlay, {
wrapper = mount(ImageDiffOverlay, {
store,
parentComponent: {
data() {
......
......@@ -62,7 +62,10 @@ describe('badge helper', () => {
});
it('should add badge classes', () => {
expect(buttonEl.className).toContain('badge badge-pill');
const classes = buttonEl.className.split(' ');
expect(classes).toEqual(
expect.arrayContaining(['design-note-pin', 'on-image', 'gl-absolute']),
);
});
it('should set the badge text', () => {
......@@ -105,7 +108,7 @@ describe('badge helper', () => {
beforeEach(() => {
containerEl.innerHTML = `
<div id="${noteId}">
<div class="badge hidden">
<div class="design-note-pin hidden">
</div>
</div>
`;
......@@ -116,7 +119,7 @@ describe('badge helper', () => {
badgeNumber,
},
});
avatarBadgeEl = containerEl.querySelector(`#${noteId} .badge`);
avatarBadgeEl = containerEl.querySelector(`#${noteId} .design-note-pin`);
});
it('should update badge number', () => {
......
......@@ -37,14 +37,16 @@ describe('domHelper', () => {
discussionEl = document.createElement('div');
discussionEl.innerHTML = `
<a href="#" class="image-diff-avatar-link">
<div class="badge"></div>
<div class="design-note-pin"></div>
</a>
`;
domHelper.updateDiscussionAvatarBadgeNumber(discussionEl, badgeNumber);
});
it('should update avatar badge number', () => {
expect(discussionEl.querySelector('.badge').textContent).toEqual(badgeNumber.toString());
expect(discussionEl.querySelector('.design-note-pin').textContent).toEqual(
badgeNumber.toString(),
);
});
});
......@@ -54,13 +56,15 @@ describe('domHelper', () => {
beforeEach(() => {
discussionEl = document.createElement('div');
discussionEl.innerHTML = `
<div class="badge"></div>
<div class="design-note-pin"></div>
`;
domHelper.updateDiscussionBadgeNumber(discussionEl, badgeNumber);
});
it('should update discussion badge number', () => {
expect(discussionEl.querySelector('.badge').textContent).toEqual(badgeNumber.toString());
expect(discussionEl.querySelector('.design-note-pin').textContent).toEqual(
badgeNumber.toString(),
);
});
});
......
......@@ -15,9 +15,9 @@ describe('ImageDiff', () => {
<div class="js-image-frame">
<img src="${TEST_HOST}/image.png">
<div class="comment-indicator"></div>
<div id="badge-1" class="badge">1</div>
<div id="badge-2" class="badge">2</div>
<div id="badge-3" class="badge">3</div>
<div id="badge-1" class="design-note-pin">1</div>
<div id="badge-2" class="design-note-pin">2</div>
<div id="badge-3" class="design-note-pin">3</div>
</div>
<div class="note-container">
<div class="discussion-notes">
......@@ -335,7 +335,7 @@ describe('ImageDiff', () => {
describe('cascade badge count', () => {
it('should update next imageBadgeEl value', () => {
const imageBadgeEls = imageDiff.imageFrameEl.querySelectorAll('.badge');
const imageBadgeEls = imageDiff.imageFrameEl.querySelectorAll('.design-note-pin');
expect(imageBadgeEls[0].textContent).toEqual('1');
expect(imageBadgeEls[1].textContent).toEqual('2');
......
......@@ -39,4 +39,72 @@ describe('Design note pin component', () => {
createComponent({ position: null });
expect(wrapper.element).toMatchSnapshot();
});
it('applies `on-image` class when isOnImage is true', () => {
createComponent({ isOnImage: true });
expect(wrapper.find('.on-image').exists()).toBe(true);
});
it('applies `draft` class when isDraft is true', () => {
createComponent({ isDraft: true });
expect(wrapper.find('.draft').exists()).toBe(true);
});
describe('size', () => {
it('is `sm` it applies `small` class', () => {
createComponent({ size: 'sm' });
expect(wrapper.find('.small').exists()).toBe(true);
});
it('is `md` it applies no size class', () => {
createComponent({ size: 'md' });
expect(wrapper.find('.small').exists()).toBe(false);
expect(wrapper.find('.medium').exists()).toBe(false);
});
it('throws when passed any other value except `sm` or `md`', () => {
jest.spyOn(console, 'error').mockImplementation(() => {});
createComponent({ size: 'lg' });
// eslint-disable-next-line no-console
expect(console.error).toHaveBeenCalled();
});
});
describe('ariaLabel', () => {
describe('when value is passed', () => {
it('overrides default aria-label', () => {
const ariaLabel = 'Aria Label';
createComponent({ ariaLabel });
const button = wrapper.find('button');
expect(button.attributes('aria-label')).toBe(ariaLabel);
});
});
describe('when no value is passed', () => {
it('shows new note label as aria-label when label is absent', () => {
createComponent({ label: null });
const button = wrapper.find('button');
expect(button.attributes('aria-label')).toBe('Comment form position');
});
it('shows label position as aria-label when label is present', () => {
const label = 1;
createComponent({ label, isNewNote: false });
const button = wrapper.find('button');
expect(button.attributes('aria-label')).toBe(`Comment '${label}' position`);
});
});
});
});
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment