AWolf81 commented 5 years ago

Related Issue:

243

Summary: Added Nolt.io. It opens a new Electron window from help/Feedback or from the UI. If the user minimizes the second screen and clicks the the feedback button again. It won't open a new feedback screen. It displays the same window.

Todos:

[x] Change Nolt board to a board from @joshwcomeau (if it's OK to use Nolt)
[x] Change DetectActive to a new component DetectHover or check if changing from span to div is possible. Styling with position: fixed is not on Wrapper, at the moment. Inlined onMouseEnter/onMouseLeave - could be refactored later. Same code as in SettingsButton.
[x] Change Nolt in app opening to open in browser as there are security concerns for login in desktop apps or is there a way to show that it's OK to use Twitter/Google login? See this tweet from Wesbos.
[x] Improve activation behaviour. Disable enter event on Wrapper as it triggeres the animation before reaching the button.

Things to discuss:

Feedback button position in lower right corner OK? Or should we place it at a different location
Feedback button styling OK? Coloring like the settings button.
Is it OK to use Nolt? I haven't used it before. I've googled a lot and that's the only service I've found that is free. But there are probably other good services. I like Nolt as styling is a bit customizable and also integrates Github (not sure how good the Github integration is but it would be great if we could create new issues from a feedback post).
It would be nice to display the most recent posts/comments on hover of the feedback button. Not sure if this is possible - I have to check the API of Nolt.
Display a number on the feedback button if there are new posts/comments (if possible)
Do we have to update privacy policy? I think we should add a note that Nolt is a data processor with a link to their privacy policy.

Screenshots/GIFs: Application menu grafik

New window grafik

Feedback button screenrecording_feedback_button_animation_new

codecov[bot] commented 5 years ago

Codecov Report

Merging #336 into master will decrease coverage by 0.18%. The diff coverage is 6%.

@@            Coverage Diff             @@
##           master     #336      +/-   ##
==========================================
- Coverage   20.92%   20.74%   -0.19%     
==========================================
  Files         246      248       +2     
  Lines        3895     3944      +49     
  Branches      389      391       +2     
==========================================
+ Hits          815      818       +3     
- Misses       2801     2845      +44     
- Partials      279      281       +2

Impacted Files	Coverage Δ
src/config/app.js	`100% <ø> (ø)`	:arrow_up:
src/components/App/App.js	`0% <0%> (ø)`	:arrow_up:
src/components/ApplicationMenu/ApplicationMenu.js	`0% <0%> (ø)`	:arrow_up:
src/components/FeedbackButton/FeedbackButton.js	`0% <0%> (ø)`
src/components/FeedbackButton/index.js	`0% <0%> (ø)`
src/components/DetectActive/DetectActive.js	`0% <0%> (ø)`	:arrow_up:
src/services/shell.service.js	`24% <20%> (-6%)`	:arrow_down:

AWolf81 commented 5 years ago

@melanieseltzer Thanks for your review. I'll look into it later today.

I like the minimal design of just the icon but you're right a text would be good. But I won't add it to the UI. Maybe just adding a tooltip would be better. So by hovering the user knows what it will do.

Also with the small icon I think it's not that critical if it's above other controls.

No, there was no decision against Doorbell. I just decided against it as there is no free plan. The free is just a 7 day free trial. I would start with Nolt. I think it's also no problem to switch this later to a differnet service if there is a better one but I think Nolt is exactly what we need.

j-f1 commented 5 years ago

How about sliding the text out on hover? Google Docs does something similar with the explore button.

melanieseltzer commented 5 years ago

No, there was no decision against Doorbell. I just decided against it as there is no free plan. The free is just a 7 day free trial.

Ah makes sense. Yeah Nolt seems decent enough.

How about sliding the text out on hover? Google Docs does something similar with the explore button.

I like this a lot ^

j-f1 commented 5 years ago

Here’s a quick mockup of a possible button style: https://beta.observablehq.com/d/c0a37d385715c706

AWolf81 commented 5 years ago

I also like the slide out feedback text. I'll add it to Guppy later today.

Here is a demo of the button that I'll add. I like it as it is. Should I improve anything? I wanted to have some transparent parts but that wasn't working. The icon background needs to be a solid color as it hides the text. The slider could be a bit transparent but with text below it looks weird.

AWolf81 commented 5 years ago

The updated animation should be OK. See updated screenrecording.

I've also changed the opening of Nolt to open in browser as I think that's better - see todo list item about a tweet with a security concern about login in a desktop app. I've kept the code for windowOpen so we could re-add that if it's OK to have a login in app. I think it would be OK to open in Guppy as the user have to trust Guppy as it can do anything on the computer - so by starting the executable the user needs to be sure that he downloaded Guppy from the right place (with-out malicious code etc.). Maybe a Virus check & checking the MD5/SHA256 would help to check that it's safe to execute - we should add these checksums to future releases.

@joshwcomeau I think just switching Nolt board to a board that you've created is open. If you're OK with Nolt, can you please create a new board and let me know the url? We also need to create some starting posts - similar to what I have added in the example board.

@idoberko2 it would be awesome if you could review the feedback animation code that I've added here as you did a lot for mirgration to React-spring. I think the code is OK but I'm pretty new to Styled-Components & React-Spring.

AWolf81 commented 5 years ago

@melanieseltzer no problem, thanks for reviewing. I'm busy with the export-to-codesandbox feature but I did some progress locally (converted logic to a saga & added unit tests). Will push an update soon.

I've tried hard to get the rounded border from the beginning but I haven't found a solution. It always scaled the radius. If we find a way to get the radius from the beginning it's OK to change. I think we could also improve the spring speed as during leave animation the slider is almost faster than the box - that's also better in the demo as this couldn't happen there as the slider & text were tied to each other.

The demo is animating the padding and I wanted to only use scale & transition to get a good performance. I think the performance of the demo is OK but just using transform will be better.

Yes, this is ready to be merged. We just have to wait for the feedback from @joshwcomeau as he should create the Nolt board. Then I'll change the link to that board.

AWolf81 commented 5 years ago

@joshwcomeau Thanks for your review.

I've added everything. Also the animation is improved - the clipping happens now all the time and not just at the end of the animation. Disapearing of the text is still a bit fast but I think it's OK.

screenrecording_feedbackbutton_latest

For the Github integration I missed that this is only available for a Pro account. So for now we don't have it but I think that's OK - we probably just have to copy some posts to Github issues and add a link in the Nolt post.

Would be great if you could add first posts to Nolt. So the board is not empty.

joshwcomeau / guppy

In-app feedback with Nolt.io #336

243

Codecov Report