feat: Relates to #128. Add feedback button that links to Fether Github new issue #319

ltfschoen · 2018-12-20T16:58:21Z

Usage: Click the feedback icon shown in the bottom right corner of the Accounts page and it'll open the Fether Github new issue page

SVG icon used: https://www.svgrepo.com/svg/27566/message

Screenshot:

…b new issue

Tbaut · 2018-12-20T20:13:31Z

Simple but nice, either it's my screen or the black of the icon is more black than other icon (such as the +)?
Can we make sure to add a tooltip with something along the side "Give feedback on Github" so that users don't have to click to know what's going on?

amaury1093 · 2018-12-20T20:17:10Z

Nice! As per #158, we're trying to move to Semantic UI. Could you maybe use one of the icons from https://react.semantic-ui.com/elements/icon/, if they are nice enough?

ltfschoen · 2018-12-20T20:50:05Z

@amaurymartiny I've used Semantic UI icons before somewhere. I just tried replacing the existing icons with them as follows but I can't get anything to appear so far:

import { Icon } from 'semantic-ui-react';

<Icon link name='question circle outline'/>

Also tried the Semantic-UI tooltip https://semantic-ui.com/modules/popup.html suggested by @Tbaut and wrapping the contents of the Feedback component as follows, but can't get the tooltip to display above the account cards (tried changing z-index and so forth unsuccessfully so far)

<div className='feedback-icon ui icon' data-tooltip='Give feedback on Github'>

Tbaut · 2018-12-21T09:01:04Z

What about something even more obvious (and what I am used to see elsewhere) as I fear a simple icon is not completely clear:

Wait for some feedback (:confused: / :+1: ?) before implementing it.

ltfschoen · 2018-12-21T09:31:26Z

@Tbaut Great idea, much better than an icon that may have people guessing as to what it means

ltfschoen · 2018-12-21T12:12:10Z

@Tbaut I've pushed a commit with the changes.

Screenshot:

packages/fether-react/src/Feedback/Feedback.js

packages/fether-react/src/Accounts/AccountsList/AccountsList.js

Tbaut

Looks good :)

ltfschoen · 2018-12-23T10:45:43Z

@amaurymartiny I've pushed commits that address your latest review comments

amaury1093 · 2018-12-31T12:48:50Z

packages/fether-react/src/Accounts/AccountsList/Feedback/Feedback.js

+};
+
+export const Feedback = () => (
+  <div className='feedback' onClick={openFeedbackLink}>


This could actually be<a href="" target="_blank" />, just to remove the cursor:pointer.

@amaurymartiny i've pushed a commit that changes it to use <a href... instead. i've also removed the hand pointer and replaced with default arrow

amaury1093 · 2018-12-31T12:50:28Z

I just tested on mac, and it shows a padding below the feedback button. @ltfschoen Can you try on yours? Apart from this nit I'm ready to merge the PR.

ltfschoen · 2018-12-31T15:02:30Z

@amaurymartiny that's strange, it doesn't have the padding on mine (see screenshot below), so i'll have to look into that.

if i go into Developer Tools and add margin-bottom: 10px; (see screenshot below) then it looks similar to what you're seeing, but it shows shadow below my feedback button whereas there isn't any below mine.

i'm using Electron Version 2.0.3 (2.0.3) and Chrome 71.0.3578.80 (Official Build) (64-bit)

ltfschoen · 2018-12-31T15:10:38Z

@amaurymartiny i have to add margin-bottom: 10px for it to add that bottom padding that you're seeing, except you don't have any bottom shadow for some reason.

i'm using the latest Chrome Version 71.0.3578.98 (Official Build) (64-bit) and Electron Version 2.0.3 (2.0.3)

amaury1093 · 2019-01-02T09:35:55Z

After some quick testing, I think it only happens when you have only 1 account (and related to the min-height property on .window).

It doesn't bother me that much actually, so I'll just let @Tbaut have a last look if he's okay with this, if yes, we can merge.

Note: what actually bothers me a little bit more is the cursor: default on hover. Any reason why not to keep the default <a> behavior?

ltfschoen · 2019-01-02T10:40:28Z

@amaurymartiny Sorry, I misinterpreted the following previous comment. I'll change back to default behaviour

just to remove the cursor:pointer

ltfschoen · 2019-01-02T10:47:33Z

@amaurymartiny Yes you're correct, I temporarily changed it to just show one account in AccountList.js (i.e. {new Array(accountsList[0]).map(address => () and it shows the feedback button offset from the bottom. I'll see if I can fix that now

Tbaut · 2019-01-02T11:15:02Z

That'd be great to fix this little offset to keep the screen consistent.

…nt list length is 0 or 1

…h is over 1

ltfschoen · 2019-01-02T11:41:53Z

@amaurymartiny @Tbaut I've pushed some commits that fixes the offset that occurs when the account list length is undefined, 0 or 1

Tbaut · 2019-01-02T21:48:22Z

The hack works, I'm good with how it looks if @amaurymartiny is fine with the hack :)

amaury1093

Not my favorite hack, but it's alright. It should be cleaner once we switch to styled-components.

feat: Relates to #128. Add feedback button that links to Fether Githu…

0f32c8f

…b new issue

ltfschoen added 2 commits December 21, 2018 15:09

review-fix: Convert feedback button into div protruding from bottom

bd01df5

Merge branch 'master' into luke-128-feedback-button

59918cf

fix: Move feedback button slightly to left

0af1d07

amaury1093 reviewed Dec 21, 2018

View reviewed changes

packages/fether-react/src/Feedback/Feedback.js Outdated Show resolved Hide resolved

packages/fether-react/src/Accounts/AccountsList/AccountsList.js Outdated Show resolved Hide resolved

Tbaut approved these changes Dec 21, 2018

View reviewed changes

Tbaut added the A5-grumble label Dec 21, 2018

ltfschoen added 2 commits December 23, 2018 18:38

review-fix: Move Feedback component into AccountsList

d408058

review-fix: Switch to more performant way to call function

0020a58

amaury1093 added A8-looksgood and removed A5-grumble labels Dec 31, 2018

amaury1093 reviewed Dec 31, 2018

View reviewed changes

ltfschoen added 2 commits January 1, 2019 02:23

refactor: Change to link and use default arrow instead of pointer hand

49453b4

review-fix: Add no bottom margin or padding

b6b4295

review-fix: Change feedback button on hover cursor back to default hand

8a35fd8

amaury1093 approved these changes Jan 2, 2019

View reviewed changes

ltfschoen added 2 commits January 2, 2019 22:35

review-fix: Hack to fix feedback button offset that occurs when accou…

ac06d7f

…nt list length is 0 or 1

fix: Shift feedback button down slightly more when account list lengt…

051b82a

…h is over 1

Merge branch 'master' into luke-128-feedback-button

f5fea76

amaury1093 approved these changes Jan 3, 2019

View reviewed changes

amaury1093 merged commit e7b7ee4 into master Jan 3, 2019

amaury1093 deleted the luke-128-feedback-button branch January 3, 2019 10:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Relates to #128. Add feedback button that links to Fether Github new issue #319

feat: Relates to #128. Add feedback button that links to Fether Github new issue #319

ltfschoen commented Dec 20, 2018 •

edited

Loading

Tbaut commented Dec 20, 2018

amaury1093 commented Dec 20, 2018

ltfschoen commented Dec 20, 2018 •

edited

Loading

Tbaut commented Dec 21, 2018

ltfschoen commented Dec 21, 2018

ltfschoen commented Dec 21, 2018 •

edited

Loading

Tbaut left a comment

ltfschoen commented Dec 23, 2018

amaury1093 Dec 31, 2018

ltfschoen Dec 31, 2018

amaury1093 commented Dec 31, 2018 •

edited

Loading

ltfschoen commented Dec 31, 2018

ltfschoen commented Dec 31, 2018 •

edited

Loading

amaury1093 commented Jan 2, 2019

ltfschoen commented Jan 2, 2019

ltfschoen commented Jan 2, 2019 •

edited

Loading

Tbaut commented Jan 2, 2019

ltfschoen commented Jan 2, 2019

Tbaut commented Jan 2, 2019

amaury1093 left a comment

feat: Relates to #128. Add feedback button that links to Fether Github new issue #319

feat: Relates to #128. Add feedback button that links to Fether Github new issue #319

Conversation

ltfschoen commented Dec 20, 2018 • edited Loading

Tbaut commented Dec 20, 2018

amaury1093 commented Dec 20, 2018

ltfschoen commented Dec 20, 2018 • edited Loading

Tbaut commented Dec 21, 2018

ltfschoen commented Dec 21, 2018

ltfschoen commented Dec 21, 2018 • edited Loading

Tbaut left a comment

Choose a reason for hiding this comment

ltfschoen commented Dec 23, 2018

amaury1093 Dec 31, 2018

Choose a reason for hiding this comment

ltfschoen Dec 31, 2018

Choose a reason for hiding this comment

amaury1093 commented Dec 31, 2018 • edited Loading

ltfschoen commented Dec 31, 2018

ltfschoen commented Dec 31, 2018 • edited Loading

amaury1093 commented Jan 2, 2019

ltfschoen commented Jan 2, 2019

ltfschoen commented Jan 2, 2019 • edited Loading

Tbaut commented Jan 2, 2019

ltfschoen commented Jan 2, 2019

Tbaut commented Jan 2, 2019

amaury1093 left a comment

Choose a reason for hiding this comment

ltfschoen commented Dec 20, 2018 •

edited

Loading

ltfschoen commented Dec 20, 2018 •

edited

Loading

ltfschoen commented Dec 21, 2018 •

edited

Loading

amaury1093 commented Dec 31, 2018 •

edited

Loading

ltfschoen commented Dec 31, 2018 •

edited

Loading

ltfschoen commented Jan 2, 2019 •

edited

Loading