search

joshwcomeau / guppy

🐠A friendly application manager and task runner for React.js
ISC License
3.27k stars 154 forks source link

Issues with CircularOutline / StrokeButton #225

Closed superhawk610 closed 5 years ago

superhawk610 commented 6 years ago

Describe the bug After a dependency install is initiated by clicking Add To Project, all other Add To Project buttons lose their green border. Strangely, this only seems to occur when installing the first search result (see screenshots).

To Reproduce

  1. Search for a dependency.
  2. Click Add To Project on the first search result.
  3. Note that all other Add To Project button borders vanish.

Expected behavior Search results for other dependencies shouldn't be affected.

Screenshots Vanishing border when installing first result

disappearing_outline

Borders remain when installing a subsequent result

not_disappearing_outline

Environment (please complete the following information):

joshwcomeau commented 6 years ago

Yeah this is really weird. I spent some time looking into this a couple months back.

The issue is that our button outlines are really unconventional; they're SVG paths. We do that rather than just using borderRadius because gradient-borders don't look at all like what I want.

~It just occurred to me, though, that probably a better solution is to just have a div with a background gradient, and 2px of padding, with an opaque white child sitting within it. To simulate the effect of a border.~

~This would fix this issue (as well as #187), and would make things quite a bit easier to understand... but it's also a fair bit of work since we'd need to come up with a new abstraction.~

EDIT: Realized that won't work, since we'd lose the ability to see through inside the outline; our sidebar icons have some padding between the outline and the icon, and it's in front of a gradient background, so that wouldn't work.

Although, this is only an issue with stroke-type buttons, and we should really be using a fill button here (I believe the UX pattern I'd like for us to adopt is that stroke buttons are informational and don't have side-effects, don't affect the filesystem, etc. Fill buttons are for performing actions.)

If we do that, it should fix this issue, and also be a step in the right direction for #227 (I'll add more thoughts there)

AWolf81 commented 6 years ago

I did a quick test with FillButton and the appearance is consistent. I found the location that's causing the outline issue: If I'm removing the passed props color1 and color2 from line 118 & 119 from AddDependencySearchResult then the border is correctly displayed with the default props. So the issue must be inside StrokeButton or CircularOutline but I couldn't find a problem.

It's really a weird issue. If I'm toggeling the css text color of the StrokeButton in developer console to default black and back to green the outline will be visible again. Not sure if it will help to debug the issue - I couldn't find anything problematic in the css.

It would be nice to fix. Changing to FillButton fixes the Add Dependency problem but we could get the issue at another location if we're using StrokeButton.

joshwcomeau commented 6 years ago

It would be nice to fix. Changing to FillButton fixes the Add Dependency problem but we could get the issue at another location if we're using StrokeButton.

Yeah. It's strange that we haven't seen it elsewhere, it's not clear exactly what the repro conditions are.

I'm guessing this is a Chrome issue, and it may be fixed in future releases of Electron. So while I'd definitely like to find a solution (likely involving a different implementation of CircularOutline?), I also don't know if it's the best use of our time.

joshwcomeau commented 6 years ago

Ok, this is causing more issues.

In our new project-configuration modal, we have many icons, and each icon requires additional render cycles. All of this happens while the modal is opening, which means the animation is choppy and awful

I think the solution could involve clip-path. What if it was a background gradient, but then CSS clip-path was used to "cut out" a border. It's a non-trivial problem since clip-path doesn't support rounded rectangles, so the path would have to be derived using circles and rectangles... but it's possible, and it would solve this issue.

We might have to provide a height since it won't be calculated dynamically, but we can do this (circularOutlines are used for our icons, where we already have the size, and for buttons, where we should be able to figure it out.

We'd lose the fact that the icons can "draw themselves", but we can replace it with a fade in/out animation. It's not as snazzy, but it's better than having a janky application, haha.

More info: https://stackoverflow.com/questions/31765345/how-to-round-out-corners-when-using-css-clip-path

joshwcomeau commented 5 years ago

Another issue: The icons blink quickly on mount: https://user-images.githubusercontent.com/3046542/45642429-6a010280-bab8-11e8-952f-af611493a5c5.gif