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

Basis: add hover-over and focus indication to the delete/edit/reply links for comments #4203

Closed
klonos opened this issue Nov 10, 2019 · 11 comments · Fixed by backdrop/backdrop#3333

Comments

@klonos
Copy link
Member

klonos commented Nov 10, 2019

I would like us to at least indicate hover by underlining the links, or ideally make these behave like "buttons", with a hover/focus color change.

Screen Shot 2019-11-11 at 8 41 44 am

to do

  • add text-decoration: underline to these links on hover.
  • decide if more button-like styles should be added.

PR by: @ArchanaAnand0212 backdrop/backdrop#3333

@ArchanaAnand0212
Copy link

ArchanaAnand0212 commented Sep 30, 2020

Hi, I worked on this as an Open Source Day Mentor in GHC 2020. I made the changes that creates a link on hover as suggested in to do.
I can convert it to button if I have an example of how other buttons look/behave in UI. Thanks

@stpaultim
Copy link
Member

@ArchanaAnand0212 - We may need to document this better, but if you put the line "Fixes #4203" in the first comment of a PR, it automatically links to this issue.

Please ping me in Slack to provide any other tips, based upon your experience, that we can add to our OSD documentation.

Commenting is also important, because it helps draw attention to the fact that work has been done on an issue.

@ghost
Copy link

ghost commented Sep 30, 2020

Thanks @ArchanaAnand0212! I've left a review in the PR requesting a few small changes based on our CSS coding standards.

@klonos klonos removed their assignment Sep 30, 2020
@stpaultim
Copy link
Member

stpaultim commented Oct 1, 2020

@BWPanda - I believe that your comments were addressed. Right?

I am marking this "works for me." I confirmed in a Backdrop CMS sandbox site that these links don't do anything on hover. With the PR - "delete," "edit," and "reply" all have an underline on hover.

I discovered other inconsistencies on this page and will open another issue.

@ghost
Copy link

ghost commented Oct 1, 2020

@stpaultim Yes. PR changes look good. However now I'm wondering why this is necessary in the first place - why don't these links have a hover state already. Is there a larger issue here, or is that out-of-scope for this particular issue...?

@klonos
Copy link
Member Author

klonos commented Oct 1, 2020

This is basically why: https://github.com/backdrop/backdrop/blob/1.x/core/themes/basis/css/component/small-text-components.css#L132

Not sure where else that code is being used, but it seems that it makes it so that you need to explicitly specify text-decoration: underline; for the links that you DO want links to be underlined on hover.

@ghost
Copy link

ghost commented Oct 1, 2020

Ok, #4670 is generally what I was referring to. Question now is: do we wait for feedback on that issue before continuing here, or merge this and hope it doesn't need reverting later?

@stpaultim
Copy link
Member

Ok, #4670 is generally what I was referring to. Question now is: do we wait for feedback on that issue before continuing here, or merge this and hope it doesn't need reverting later?

@BWPanda - this is a good question. I don't know the answer. It might make sense to wait and have someone look at the larger picture here and suggest a systematic solution before merging this commit.

I don't know if the solution here will be one commit or many.

@stpaultim
Copy link
Member

stpaultim commented Oct 2, 2020

Well, it looks like we are moving towards a more global solution in #4670 that MIGHT yet contradict the solution in this PR.

@bugfolder
Copy link

Code reviewed on backdrop/backdrop#3333, LGTM. (Note that its initial comment doesn't include the magic "Fixes" word to bind it to this issue.)

@quicksketch
Copy link
Member

Thanks @bugfolder for resurfacing this issue. I've merged backdrop/backdrop#3333 into 1.x and 1.26.x. Thanks so much @ArchanaAnand0212 and sorry this took so long to get merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants