Skip to content
This repository has been archived by the owner on Dec 15, 2023. It is now read-only.

feat(avatar): add clickable behavior #1176

Merged
merged 11 commits into from
Sep 19, 2023

Conversation

juliodialpad
Copy link
Contributor

@juliodialpad juliodialpad commented Sep 13, 2023

Feat (Avatar): Add clickable behavior

πŸ› οΈ Type Of Change

  • Fix
  • Feature
  • Refactoring
  • Documentation

πŸ“– Description

  • Added clickable property to dt-avatar to enable the possibility to focus and click on avatars.
  • Removed the ability to select the text within the avatar.
  • Added clickable avatar examples to variants.
  • Added documentation
  • Added tests

πŸ’‘ Context

Callbox component needs to emit an event when clicking on the avatar, also in this PR a clickable avatar was needed and a custom implementation was made, so I thought that maybe a clickable avatar is a good feature to have.

πŸ“ Checklist

  • I have reviewed my changes
  • I have added tests
  • I have added all relevant documentation
  • I have validated components with a screen reader
  • I have validated components keyboard navigation
  • I have considered the performance impact of my change
  • I have checked that my change did not significantly increase bundle size
  • I am exporting any new components or constants in the index.js in the component directory
  • I am exporting any new components or constants in the index.js in the root

πŸ“· Screenshots / GIFs

image

@github-actions
Copy link

Thanks for contributing to Dialtone Vue! Please read below for some important info regarding Vue 3 compatibility.

Currently we need to maintain two branches in Dialtone Vue, one for Vue 2, one for Vue 3.

This means you must create two PRs for every feature change you make. One into staging and one into staging-vue3.

Many times the change you have made in Vue 2 will be identical to the change you need to make in Vue 3. To make this easier we have made a script that can copy your changes from this branch to a new branch off of staging-vue3 suffixed with -vue3.

run ./copy_pr_vue3.sh from the root dialtone-vue directory.

Once the new branch is created, you will need to look at your code to make sure it still makes sense and test that your changes all work in vue 3. If everything is good you can push it and create a PR into staging-vue3.

It is a required check for every PR to have a corresponding branch called yourbranch-vue3 so that we do not forget to do it. In the special case that you need to make a change to vue2 without making it in vue3, you can set the label vue2-only on this PR.

I got "commit SHA is a merge but no -m option was given."

This happens if there are merge commits in your branch. It's no problem, you can simply skip them with git cherry-pick --skip. We don't want to copy merge commits to the Vue 3 branch.

What if I make more changes to my vue 2 branch after running ./copy_pr_vue3.sh?

You can copy these to the existing -vue3 branch by running the script with a git SHA param like so:

./copy_pr_vue3.sh 2a78db7

where 2a78db7 is the last commit from your branch that was copied to the other branch (all commits after this will be copied)

If it's just one or two commits, you may prefer to just manually use git cherry-pick which will work fine as well.

What if I get a conflict?

It's possible to get a conflict when running copy_pr_vue3.sh as there are differences in Vue 2 and Vue 3 code. If this happens you can manually fix the conflict, commit it and do git cherry-pick --continue.

@github-actions
Copy link

βœ”οΈ Deploy Preview ready!
😎 Browse the preview: https://vue.dialpad.design/deploy-previews/pr-1176/
πŸ”¨ If you experience an SSL issue then wait 2 minutes and try again.

@iropolo
Copy link
Contributor

iropolo commented Sep 13, 2023

a11y test fail on

[TEST] FAIL browser: chromium components/avatar/avatar.stories.js

[TEST] β”‚ 0 β”‚ 'aria-command-name' β”‚ 'serious' β”‚ 'Ensures every ARIA button, link and menuitem has an accessible name' β”‚ 1 β”‚

@iropolo
Copy link
Contributor

iropolo commented Sep 13, 2023

Cool, looks good, I would revert the update in recipes/list_items/contact_info/contact_info.vue component here #1164.
And apply this update.

This is my suggested change to recipes/list_items/contact_info/contact_info.vue component:
As you will see, Im reverting
<button> tag to <div>
and
<span> to <div>

Also it would probably necessary to remove avatarAriaLabel prop.

    <template #left>
      **<div**
        v-if="showAvatar"
        class="d-ps-relative d-bgc-transparent d-baw0 d-c-pointer"
        data-qa="contact-info-left"
      >
        **<div**
          v-if="avatarList"
          class="dt-contact-info--avatars d-mrn4 d-d-flex d-fd-row"
        >
          <dt-avatar
            v-for="(avatar, index) in avatarList"
            :key="index"
            **:clickable="true"**
            :size="avatarSize"
            :seed="avatar.seed"
            :full-name="avatar.fullName"
            :image-src="avatar.src"
            :icon-name="avatarIcon"
            :overlay-icon="avatar.icon"
            :overlay-text="avatar.text"
            :avatar-class="[{ 'd-mln24': index > 0, 'd-bc-brand': !!avatar.halo }]"
            **@click="avatarClick"**
          />
        **</div>**
        <dt-avatar
          v-else
          **:clickable="true"**
          :size="avatarSize"
          :full-name="avatarFullName"
          :image-src="avatarSrc"
          :icon-name="avatarIcon"
          :seed="avatarSeed"
          :presence="presence"
          **@click="avatarClick"**
        />
      **</div>**
    </template>

This is the final result:

image

Ping @tonydialpad here to check out this suggested change.

Copy link
Contributor

@iropolo iropolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-approving it but need to fix the a11y issue.

Looks good improvement for avatar component, feel free to update recipes/list_items/contact_info/contact_info.vue component here, or create a ticket to update it later.

It will be necessary to update also the vue3 version of it, but is still pending 1177

@github-actions github-actions bot added the visual-test-ready Add this tag when the PR is ready for visual test, to trigger GHA visual tests label Sep 13, 2023
},
]"
:tabindex="clickable ? 0 : -1"
:role="clickable ? 'button' : 'presentation'"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change the actual element instead of just setting role? Can we have some sort of indicator on hover to show it is clickable? Maybe overlay it with a dark color with some opacity?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What element should I use instead, a button?
Maybe I can show the focus ring on hover too, I was using cursor: pointer to show it is clickable on hover but maybe the opacity would work better.

@francisrupert @fede-dp please help us on this decision.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it should be button I think

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Comment on lines 346 to 350
keydown: (e) => {
if (!['Space', 'Enter'].includes(e.code)) return;
e.preventDefault();
this.$emit('click', e);
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we wouldn't have to worry about all this if the element was an actual button

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try with a button

@github-actions
Copy link

βœ”οΈ Deploy Preview ready!
😎 Browse the preview: https://vue.dialpad.design/deploy-previews/pr-1176/
πŸ”¨ If you experience an SSL issue then wait 2 minutes and try again.

@francisrupert
Copy link
Collaborator

@juliodialpad,

Sorry this got buried in my notifications.

Yes, button.

.d-avatar--clickable:hover {
  border: var(--dt-size-border-100) solid var(--dt-color-border-default);
}

.d-avatar--clickable:active {
  border-color: var(--dt-color-border-moderate);
}

I don't mind the .98 scaling for :active. Stick with that for now, and I can always change it after seeing it in play.

@github-actions
Copy link

βœ”οΈ Deploy Preview ready!
😎 Browse the preview: https://vue.dialpad.design/deploy-previews/pr-1176/
πŸ”¨ If you experience an SSL issue then wait 2 minutes and try again.

@juliodialpad
Copy link
Contributor Author

a11y test fail on

[TEST] FAIL browser: chromium components/avatar/avatar.stories.js

[TEST] β”‚ 0 β”‚ 'aria-command-name' β”‚ 'serious' β”‚ 'Ensures every ARIA button, link and menuitem has an accessible name' β”‚ 1 β”‚

Fixed, good catch!

@juliodialpad
Copy link
Contributor Author

juliodialpad commented Sep 15, 2023

Cool, looks good, I would revert the update in recipes/list_items/contact_info/contact_info.vue component here #1164. And apply this update.

This is my suggested change to recipes/list_items/contact_info/contact_info.vue component: As you will see, Im reverting <button> tag to <div> and <span> to <div>

Also it would probably necessary to remove avatarAriaLabel prop.

    <template #left>
      **<div**
        v-if="showAvatar"
        class="d-ps-relative d-bgc-transparent d-baw0 d-c-pointer"
        data-qa="contact-info-left"
      >
        **<div**
          v-if="avatarList"
          class="dt-contact-info--avatars d-mrn4 d-d-flex d-fd-row"
        >
          <dt-avatar
            v-for="(avatar, index) in avatarList"
            :key="index"
            **:clickable="true"**
            :size="avatarSize"
            :seed="avatar.seed"
            :full-name="avatar.fullName"
            :image-src="avatar.src"
            :icon-name="avatarIcon"
            :overlay-icon="avatar.icon"
            :overlay-text="avatar.text"
            :avatar-class="[{ 'd-mln24': index > 0, 'd-bc-brand': !!avatar.halo }]"
            **@click="avatarClick"**
          />
        **</div>**
        <dt-avatar
          v-else
          **:clickable="true"**
          :size="avatarSize"
          :full-name="avatarFullName"
          :image-src="avatarSrc"
          :icon-name="avatarIcon"
          :seed="avatarSeed"
          :presence="presence"
          **@click="avatarClick"**
        />
      **</div>**
    </template>

This is the final result:

image Ping @tonydialpad here to check out this suggested change.

Created a ticket to solve this later: https://dialpad.atlassian.net/browse/DLT-1230

@github-actions
Copy link

βœ”οΈ Deploy Preview ready!
😎 Browse the preview: https://vue.dialpad.design/deploy-previews/pr-1176/
πŸ”¨ If you experience an SSL issue then wait 2 minutes and try again.

@juliodialpad
Copy link
Contributor Author

Updated please re-review.

@juliodialpad juliodialpad removed the visual-test-ready Add this tag when the PR is ready for visual test, to trigger GHA visual tests label Sep 15, 2023
@github-actions
Copy link

βœ”οΈ Deploy Preview ready!
😎 Browse the preview: https://vue.dialpad.design/deploy-previews/pr-1176/
πŸ”¨ If you experience an SSL issue then wait 2 minutes and try again.

Copy link
Contributor

@braddialpad braddialpad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great thanks!

The button moves the slightest amount when enabling clickable due to the scale change. Not really a big deal but could maybe cause a misalignment issue in some cases?

2023-09-15 16 27 08

@github-actions github-actions bot added the visual-test-ready Add this tag when the PR is ready for visual test, to trigger GHA visual tests label Sep 15, 2023
@juliodialpad
Copy link
Contributor Author

Great thanks!

The button moves the slightest amount when enabling clickable due to the scale change. Not really a big deal but could maybe cause a misalignment issue in some cases?

2023-09-15 16 27 08

Yeah, it might, I have two options here:

Making the border inset or adding the border to the non-clickable version which might cause a misalignment too.

Any advice here? @francisrupert

Copy link
Contributor

@iropolo iropolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks!!

@github-actions
Copy link

βœ”οΈ Deploy Preview ready!
😎 Browse the preview: https://vue.dialpad.design/deploy-previews/pr-1176/
πŸ”¨ If you experience an SSL issue then wait 2 minutes and try again.

@juliodialpad
Copy link
Contributor Author

Added the border by default to the clickable avatar and just change the color on :hover and :active, this will increase the size of avatars which I don't think it's to much of an issue because this way the size will not change.

I was thinking on adding it to all the avatar, but that'd increase the size of all the avatars across the app and that'd create a lot of issues with QA

@github-actions
Copy link

βœ”οΈ Deploy Preview ready!
😎 Browse the preview: https://vue.dialpad.design/deploy-previews/pr-1176/
πŸ”¨ If you experience an SSL issue then wait 2 minutes and try again.

@juliodialpad juliodialpad merged commit 2b80018 into staging Sep 19, 2023
11 checks passed
@juliodialpad juliodialpad deleted the feat/avatar/add-clickable-behavior branch September 19, 2023 01:25
juliodialpad pushed a commit that referenced this pull request Sep 20, 2023
# [2.97.0](v2.96.0...v2.97.0) (2023-09-20)

### Bug Fixes

* add virtual "prompt" for visual tests on PR ([#1192](#1192)) ([4a41d02](4a41d02))
* **Avatar:** change avatar seed algorithm to sync with android ([#1182](#1182)) ([53a09f3](53a09f3))
* **Contact Info, Feed Item Row, Item Layout:** alignment issues ([#1186](#1186)) ([bdaa355](bdaa355))
* feed pill not exported ([#1180](#1180)) ([792b838](792b838))
* **Item Layout:** selected slot alignment ([#1194](#1194)) ([09c37c4](09c37c4))
* **Recipe Callbar Button With Popover:** change event name and sync open state ([#1183](#1183)) ([b91dc02](b91dc02))

### Features

* **Avatar:** add clickable behavior ([#1176](#1176)) ([2b80018](2b80018))
* **Callbox:** add clickable behavior ([#1196](#1196)) ([bf5ac2b](bf5ac2b))
@github-actions
Copy link

πŸŽ‰ This PR is included in version 2.97.0 πŸŽ‰

The release is available on GitHub release

Your semantic-release bot πŸ“¦πŸš€

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
released visual-test-ready Add this tag when the PR is ready for visual test, to trigger GHA visual tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants