Skip to content

[Fix] Adds scrolling to description popover#170

Merged
n1474335 merged 3 commits intogchq:masterfrom
mattnotmitt:bug/text-overflow
Aug 1, 2017
Merged

[Fix] Adds scrolling to description popover#170
n1474335 merged 3 commits intogchq:masterfrom
mattnotmitt:bug/text-overflow

Conversation

@mattnotmitt
Copy link
Copy Markdown
Collaborator

Changes data-trigger to focus so scrolling works, sets the max height and enables scrollbar.
Fixes #137.

Changes data-trigger to focus so scrolling works and sets max height.
@n1474335
Copy link
Copy Markdown
Member

Thanks for taking a look at this. There are a couple of undesirable behaviours associated with it at the moment which I can't quite iron out.

  • It would be nice if popovers were still triggered by hover instead of having to focus them first. I find that most people don't notice or read the descriptions otherwise. Happy to debate this though - are the popups annoying for anyone? Can we think of a nicer way of displaying the descriptions that gets in people's faces, but not in a frustrating way?
  • Adding overflow-y: auto to the .popover class makes the arrow tag fall out the bottom instead of being displayed on the side next to the operation it refers to. The fix for this is to add the overflow to the .popover-content class instead, but this doesn't seem to work as expected.

@mattnotmitt
Copy link
Copy Markdown
Collaborator Author

You can't have hover and scrolling at the same time because as soon as you stop hovering over the element it disappears, and if you try scrolling when you're hovering it just scrolls the list of operations! I'm not sure what the solution to this is though. I'll fix the overflow/arrow issue shortly, however.

@n1474335
Copy link
Copy Markdown
Member

I agree this is not possible without modifications to the way Bootstrap handles the hover event. There are some possible solutions here that might work: https://stackoverflow.com/questions/7703878/how-can-i-hold-twitter-bootstrap-popover-open-until-my-mouse-moves-into-it

@mattnotmitt
Copy link
Copy Markdown
Collaborator Author

mattnotmitt commented Jul 28, 2017

Right, I've fixed the arrow issue. When fixing this, I was thinking that maybe we should set the max-height to 80 or 90%, I think it looks less cramped than having it flow all the way from the top to the bottom of the screen. I've set this at 90% in the latest commit, so maybe I could get a few second opinions.

In regards to the hover event issue; to use the solutions seen on SO we'd have to run it every time the list of operations updates - I'm not particularly sure how I can catch this, but the solution that I got to work best was this one. If I can figure out how to call that every time the list is redisplayed, we'll be all good on that front!

@n1474335
Copy link
Copy Markdown
Member

There is a listener for oplistcreate events and its main handler is here: https://github.com/gchq/CyberChef/blob/master/src/web/OperationsWaiter.js#L150

@n1474335 n1474335 merged commit 9ee0964 into gchq:master Aug 1, 2017
@n1474335
Copy link
Copy Markdown
Member

n1474335 commented Aug 1, 2017

Thanks very much for this. I've made some further modifications to fix some edge cases but I think it gives the correct behaviour now.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants