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

Set the mouse cursor to pointer on hover for Person Card enabled mgt-… #2652

Merged

Conversation

agaskelluk
Copy link
Contributor

Closes #2651

PR Type

Bugfix

Description of the changes

The mgt-person component should have a cursor of type pointer display when mouse over event occurs on the mgt-person component if it is equipped with either a Person Card Hover or Person Card Click.
I have updated the mgt-person.scss styles to include this property.
Current workaround is to set your own mouse pointer style via custom css every time.

PR checklist

  • Project builds (yarn build) and changes have been tested in at least two supported browsers (Edge + non-Chromium based browser)
  • All public APIs (classes, methods, etc) have been documented following the jsdoc syntax
  • Stories have been added and existing stories have been tested
  • Added appropriate documentation. Docs PR:
  • License header has been added to all new source files (yarn setLicense)
  • Contains NO breaking changes

Other information

@agaskelluk agaskelluk requested a review from a team as a code owner August 10, 2023 12:56
@microsoft-github-policy-service
Copy link
Contributor

Thank you for creating a Pull Request @agaskelluk.

This is a checklist for the PR reviewer(s) to complete before approving and merging this PR:

  • I have verified a documentation PR has been linked and is approved (or not applicable)
  • I have ran this PR locally and have tested the fix/feature
  • I have verified that stories have been added to storybook (or not applicable)
  • I have tested existing stories in storybook to verify no regression has occured
  • I have tested the solution in at least two browsers (Edge + 1 non-Chromium based browser)

Copy link
Collaborator

@Mnickii Mnickii left a comment

Choose a reason for hiding this comment

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

Works great!

@sebastienlevert
Copy link
Contributor

Thanks for the PR @agaskelluk! As discussed in the corresponding issue, after this issue went through design review, we believe hover effects shouldn't have a pointer but only the click experience. Adding @Mnickii to see with you what are the options to achieve this expected behavior. Thanks!

@agaskelluk
Copy link
Contributor Author

Thanks for the PR @agaskelluk! As discussed in the corresponding issue, after this issue went through design review, we believe hover effects shouldn't have a pointer but only the click experience. Adding @Mnickii to see with you what are the options to achieve this expected behavior. Thanks!

@sebastienlevert @Mnickii I appreciate the update. However this is going to be confusing for any users who are used to the OOTB People WP.

If you check the behaviour of the OOTB People WP in SP, you will see that the cursor changes to a pointer on hover. This makes it clear that there is additional functionality available.

The same is true for the OOTB People WP in Teams. If you hover over anyone's name in chat for example, you will see that the cursor changes to a pointer before bringing up the people card.

Its not really a problem for me to implement in a custom fashion, I just added the style to my custom stylesheet in my Web Part SPFx solution and it works fine, but I wanted you to be aware.

@Mnickii
Copy link
Collaborator

Mnickii commented Aug 11, 2023

That's a great point @agaskelluk that the cursor changes to a pointer on hover for both people WP in SP and people WP in teams, however in a 1:1 chat messages in Teams, we see our intended behavior as per our design review where on hover on a person, the cursor does not change to a pointer.

@agaskelluk
Copy link
Contributor Author

That's a great point @agaskelluk that the cursor changes to a pointer on hover for both people WP in SP and people WP in teams, however in a 1:1 chat messages in Teams, we see our intended behavior as per our design review where on hover on a person, the cursor does not change to a pointer.

Yes I see that behaviour as well in a 1:1 chat. OK at least we are all aware. Thanks for the response.

@Mnickii
Copy link
Collaborator

Mnickii commented Aug 11, 2023

In this case, then you could specifically target the click personCardInteraction and add the pointer to that. Something like adding personCardInteractionClick: this.personCardInteraction === PersonCardInteraction.click to the renderFlyout rootClasses in mgt-person.ts then setting the pointer like so&.personCardInteractionClick { cursor: pointer; } in person-root in mgt-person.scss

@agaskelluk
Copy link
Contributor Author

@Mnickii yes I am sure we could target the onclick event only, but IMHO this will be confusing for my users who are used to seeing feedback in the OOTB SharePoint WP onhover event, so I will stick with my customization of both the onclick and onhover events.
Initially this PR was accepted, so does that mean the changes got incorporated into master anyway? What is the status please?

@sebastienlevert
Copy link
Contributor

I'll have to lean on @agaskelluk side here. When looking at the ODSP People WP, I'd say this is the closest behavior we're replicating here. I would argue it should have a cursor pointer (same behavior in Outlook, BTW). I'm of with approving and merging this one.

Copy link
Contributor

@sebastienlevert sebastienlevert left a comment

Choose a reason for hiding this comment

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

Works great!

@sebastienlevert
Copy link
Contributor

@agaskelluk this should get merged this week. We're preparing a version so it might get publish soon. Thanks!

@gavinbarron gavinbarron merged commit 48ea18b into microsoftgraph:main Aug 15, 2023
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[BUG] mgt-person no cursor change on hover
4 participants