Applied fixes for disappearing icons. #947
Conversation
| @@ -36,45 +35,43 @@ function EditIcons({ | |||
|
|
|||
| return ( | |||
| <div className={className}> | |||
| {(hovering || (window.__isTouch && active) || forceShow) && ( | |||
There was a problem hiding this comment.
Since hovering prop is removed, this entire condition is removed because it is handled above without consideration for hovering.
| @@ -36,45 +35,43 @@ function EditIcons({ | |||
|
|
|||
| return ( | |||
| <div className={className}> | |||
| {(hovering || (window.__isTouch && active) || forceShow) && ( | |||
| <Container> | |||
There was a problem hiding this comment.
Also, none of this code has changed. Only the containing {(hovering || .......... && ( ... )} thing was removed.
| @@ -179,10 +173,10 @@ class Entry extends React.PureComponent { | |||
| {state === '' && ( | |||
| <Right> | |||
| {isMainModule ? ( | |||
| <span style={{ opacity: hovering ? 1 : 0 }}>main</span> | |||
| <span className="mainIndicator">main</span> | |||
There was a problem hiding this comment.
Always rendered, but only visible (via CSS) on :hover of EntryContainer.
| ) : ( | ||
| <EditIcons | ||
| hovering={hovering} | ||
| className="actionIcons" |
There was a problem hiding this comment.
Always rendered, but only visible (via CSS) on :hover of EntryContainer.
| @@ -78,6 +78,19 @@ export const getContainerStyles = props => { | |||
|
|
|||
| export const EntryContainer = styled.span` | |||
| ${props => getContainerStyles(props)}; | |||
| .actionIcons, | |||
|
@alexvcasillas suggested on Twitter to work it out with I am not sure how this will affect touch screens.
|
|
You're welcome for the suggestion, glad to read that you solved it.
Regarding of touch screens I think this won't be an issue because
pointer-events works nice with them (been working on that for two years at
Genially and it was perfect on touch screens).
Let's hope this pass all tests and get merged 🤞🏻
El El jue, 5 jul 2018 a las 8:55, Colton Colcleasure <
[email protected]> escribió:
@alexvcasillas <https://github.com/alexvcasillas> suggested on Twitter
<https://twitter.com/alexvcasillas/status/1014754312684294144> to work it
out with pointer-events. So I toyed a bit and came up with a solution to
the jittery-ness, as well, and this change is reflected in the latest
commit.
*I am not sure how this will affect touch screens.*
[image: 2018-07-05 01 51 23]
<https://user-images.githubusercontent.com/19484365/42306932-70f2dd22-7ff6-11e8-9530-051b0862188a.gif>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#947 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AJDpgG1BFW4Z7b-z-HQkHUmcVEm_7it0ks5uDbh7gaJpZM4VDR-l>
.
--
*Alex Casillas* Software Engineer & Web Developer
🔸 https://alexvcasillas.com
🔸 https://twitter.com/alexvcasillas
🔸 https://github.com <https://github.com/alexvcasillas>/alexvcasillas
<https://github.com/alexvcasillas>
|
|
Thanks a lot for the PR! This is very nice! One thing I'm wondering about is tablets/phones. I think we should show the icons at all times for selected files, because if you're on a tablet you won't know that you can rename/create/etc files otherwise. Unless this already works thanks to the focus property? Maybe we need some testing on this. |
|
Yeah, icons should be always displayed on touch devices due to the lack of
"hover" state.
|
|
Here is the required mobile behavior:
|
|
Awesome job colshacol looks very nice 👌🏻 |
|
@CompuIves Behavior checks out on mobile. I'd gladly add some tests, but I don't really see any other tests hanging around. Could you point me in the right direction? |
|
Icons should be always visible on touch screens so it could be tapable
easily :)
|
|
@alexvcasillas I think the current behavior is great. When the user indicates they are interacting with a folder or file, actions show up. Otherwise, they are not visible. It keeps it from getting too crowded. And I have manually tested that this is the behavior, even after my changes. But for actual testing, like unit tests or E2E, I don't recall seeing any other tests sitting anywhere for me to structure mine after. |
|
Heya! I did some testing on mobile and I can't seem to get the behaviour you describe yet (the one where the current directory has icons and the file opened has icons). But it seems like it works in your screenshot, maybe I'm doing something wrong.
Sorry for that one, we're pretty bad on the testing side :(. I'm looking forward for merging this in, the new behaviour looks much more solid! Only thing I want to look out for is the icons on mobile, as that is something that people complained about in the past. If we cannot get it done with CSS we may fallback to the JS solution for mobile? |
|
What's the idea of the JS solution about? Icons are already SVG so they'll look sharp and all that stuff. Can you relate a little more about the issue are people having with mobile? |
|
People working on iPads don't want to tap a dir/file twice to do an edit to it in most cases. There are also tablet users that don't know they can rename, edit, etc. the files. Before they had to first click on the file to show the icons, and then click on the icon to edit. I solved this in JS by adding some logic (if we're on mobile and the file/dir is currently opened: show the icons). I reckon this is hard to implement in CSS, so maybe we could fallback to it when we detect a mobile device. Or maybe there are better ways? |
|
We should then detect wheter it's a touch device or not and fix the css by removing all the related :hover selectors so the problem would be easily fixed. Using styled-components this would be easy since you can interpolate JS expressions to control this within the template literal. I can take a look at this. Is styled-components a thing for this proyect or we should use regular CSS? I can give it a try if you want it but first I have to know the specs on how I'm able to solve this issue. |
|
@CompuIves It may be device specific. I can only test on my Galaxy S9 at home. (My method to manually test it is start up the server and then use Seveo to make it available externally so I can connect to it from my phone.) I am excited to merge this, as well, because it is a problem that bugs the hell out of me and I am proud to have provided a solution instead of just an issue. But I also do not want to break anybody else just to solve my annoyances, haha. I am thinking that maybe the whole thing could be fixed just by doing the inline-styling for pointer-events in |
|
Also, just saying... Setting up Zeit Now for auto build on PR would be amazing. Then we could easily share the link for others to view/test on all devices. :) |
|
We already have this: http://pr947.cs.lbogdan.tk/s/new .
|
|
Ah, very nice! So, I tried it on 3 other devices and it worked just fine. -- Of course, you have to click "Edit on CodeSandbox" first, because before that you can't perform any actions at all. |
|
I finally came to a solution that is just a few lines of CSS after spending hours investigating the issue deeply. The problem turned out to be this:
With the tooltip wrapper and inner icon components only being 22px in height, it made losing pointer status on the icon too easy. When you hovered an icon, the tooltip appeared, but it is rendered outside of this tiny component tree (in a portal) and does not play by the rules set forth in this context. Upon moving your mouse upwards off the icon, the tooltip fades and slides downwards, causing your mouse to enter that ancestral-cousin tooltip div and leave the directory/file container row div. (i.e A refresher of the original issue in action:
In order to fix it, I found A demo of the resolved issue:
In order to attempt to fix this without altering functionality, I actually got frustrated and kind of forked the repo all over again. >.> So I can't exactly push to this no-longer-existing branch. Here are my 2 commits. @CompuIves Would you be fine with me making a separate PR? |
|
Ah yes, that seems like the best solution @colshacol. Thanks for looking into this more! Could you open a new PR with that logic? Then I'll merge it in. Oh, and you can add yourself as contributor by running |
|
New PR with CSS-only solution: #964 |
Bug Fix
I have been plagued by this bug while working in a Sandbox. Basically, when hovering over a directory, the actionable icons appear. But things get weird when hovering over the icons. In most cases, I hover the icons and they disappear before I get to click, and I have to carefully re-hover the area.
I spent many hours fighting with this bug, even trying out different libraries and strategic approaches to getting it to stop. Unfortunately, nothing would work.
I got the issue to a less aggravating state with the changes reflected in this PR. (Basically converting showing/hiding of action icons to being handled by CSS purely.) The are now always rendered, but only shown if the
<EntryContainer>is in a:hoverstate.This leads to less state transitions and icons that are always visible when needed, while invisible when not needed.
Compared with the first GIF, you can see that the icons still act funky at specific hover points, but do not completely disappear and stay gone when you are hovering.
And here is a detailed preview of the remaining UI buggish-ness.