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

Change cursor to a pointer when hover over read notification #2370

Closed
wants to merge 1 commit into from

Conversation

jhadvig
Copy link
Member

@jhadvig jhadvig commented Oct 25, 2017

@jwforres PTAL

Closes #2293

@jhadvig jhadvig requested a review from jwforres October 25, 2017 12:28
@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 25, 2017
@jwforres
Copy link
Member

/assign @benjaminapetersen

@spadgett
Copy link
Member

I think the fix is to use the default (arrow) cursor when read. Is that what this does? It looks like it makes it a pointer (hand) cursor always.

@benjaminapetersen
Copy link
Contributor

So on Master I see what @spadgett mentioned, which is to use the default arrow once it has been read:

2017-10-25 11 41 34

This PR will always use the pointer:

2017-10-25 11 43 39

@jwforres can you clarify what you mentioned on this comment? Do we just want to eliminate the hover effect entirely?

@@ -39,9 +39,9 @@ notification-drawer-wrapper {
}
// the whole block is clickable, need to set pointer on all of these
// for the correct visual
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is quite right, might be that we just want to eliminate the hover (bg blue) as well as return the cursor.

@spadgett
Copy link
Member

If we switch to the default cursor, I think we'd want to remove the blue hover style as well? But I might have misunderstood @jwforres' comment.

I personally don't have a strong preference here.

@jwforres
Copy link
Member

weird, i thought i was on master when i checked it...

i would at least see what it looks like without the hover color

@jhadvig
Copy link
Member Author

jhadvig commented Oct 26, 2017

So from this comment I was in a impression we want to have to pointer cursor on unread and also read notifications.

@jhadvig
Copy link
Member Author

jhadvig commented Oct 26, 2017

Thinking about it now Im not really sure it would make sense to have the pointer on read notification. If we align on this it would be better to merge @benjaminapetersen PR
If align on the pointer for both cases the I would rather keep the changed background on the hover, just to to have a similar behavior of read and unread notifications.

@jwforres @spadgett thoughts ?

@jhadvig
Copy link
Member Author

jhadvig commented Oct 26, 2017

Closing this PR in favor of #2383
@benjaminapetersen fyi :)

@jhadvig jhadvig closed this Oct 26, 2017
@benjaminapetersen
Copy link
Contributor

Cool, think mine is gonna get closed as well. Yay discussions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants