Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DDW-852] Improve Stake Pools list view attention #2847

Merged
merged 9 commits into from Feb 14, 2022

Conversation

renanvalentin
Copy link
Contributor

@renanvalentin renanvalentin commented Feb 1, 2022

This PR separate the buttons from the search bar.

Todos

  • List view tooltip visible for first-time users
  • Improve button hover effect

Screenshots

Screen Shot 2022-02-01 at 09 38 48

Screen Shot 2022-02-01 at 16 04 49

Screen Shot 2022-02-14 at 14 05 47
Screen Shot 2022-02-14 at 14 05 53

Testing Checklist

Test Scenarios

Scenario 1: List view tooltip displayed
Given I (freshly installed | upgraded) Daedalus
When I navigate to 'Stake pools' tab
Then the list view tool tip is displayed
And the list view tool tip has the words 'List View' printed

Scenario 2: List view tooltip after hover|click,  not displayed in subsequent launch(es)
Given I (freshly installed | upgraded) Daedalus
And I navigate to 'Stake pools' tab
And the list view tool tip is displayed
And I have (hovered | clicked) on List View icon
And the list view tool tip has disappeared
When I relaunch Daedalus
Then the list view tool tip is not displayed
And upon hover the list view tool tip is displayed
And  the list view tool tip has the words 'List View' printed

Scenario 3: List view tooltip displayed in subsequent launch(es)
Given I (freshly installed | upgraded) Daedalus
And I navigate to 'Stake pools' tab
And the list view tool tip is displayed
When I relaunch Daedalus
Then the list view tool tip is still displayed
And upon (hover | click) on List view icon, the list view tool tip disappears

Review Checklist

Basics

  • PR assigned to the PR author(s)
  • input-output-hk/daedalus-dev and input-output-hk/daedalus-qa assigned as PR reviewers
  • If there are UI changes, Alexander Rukin assigned as an additional reviewer
  • All visual regression testing has been reviewed (Go to Github Actions tab -> Select Chromatic workflow -> Run on your working branch)
  • PR has appropriate labels (release-vNext, feature/bug/chore, WIP)
  • PR link is added to a Jira ticket, ticket moved to In Review
  • PR is updated to the most recent version of the target branch (and there are no conflicts)
  • PR has a good description that summarizes all changes
  • PR contains screenshots (in case of UI changes)
  • CHANGELOG entry has been added to the top of the appropriate section (Features, Fixes, Chores) and is linked to the correct PR on GitHub
  • There are no missing translations (running yarn manage:translations produces no changes)
  • Text changes are proofread and approved (Jane Wild / Amy Reeve)
  • Japanese text changes are proofread and approved (Junko Oda)
  • Storybook works and no stories are broken (yarn storybook)
  • In case of dependency changes yarn.lock file is updated

Code Quality

  • Important parts of the code are properly commented and documented
  • Code is properly typed with typescript types
  • React components are split-up enough to avoid unnecessary re-renderings
  • Any code that only works in main process is neatly separated from components

Testing

  • New feature/change is covered by acceptance tests
  • New feature/change is manually tested and approved by QA team
  • All existing acceptance tests are still up-to-date
  • New feature/change is covered by Daedalus Testing scenario
  • All existing Daedalus Testing scenarios are still up-to-date

After Review

  • Update Slack QA thread by marking it with a green checkmark

@alexander-rukin
Copy link
Contributor

alexander-rukin commented Feb 1, 2022

image

hm, this hover effect doesn't look right 😓 it should just go from 0.3 to 0.7 current problem is probably because of "< button >" usage

@alexander-rukin
Copy link
Contributor

image

@daniloprates didn't we want to have this popup "pre-opened" for the first visit?

@thedanheller
Copy link
Member

image

@daniloprates didn't we want to have this popup "pre-opened" for the first visit?

We do! @renanvalentin can we have it? Just so we make sure everyone knows the list view exists.

@renanvalentin
Copy link
Contributor Author

image @daniloprates didn't we want to have this popup "pre-opened" for the first visit?

We do! @renanvalentin can we have it? Just so we make sure everyone knows the list view exists.

@daniloprates @alexander-rukin should we do this in another ticket? Otherwise it will slight increase the scope of this one.

@renanvalentin
Copy link
Contributor Author

@alexander-rukin @daniloprates added tooltip for list view. The user has to click on the list view button in order to register "seen" event. Does it makes sense?

@alexander-rukin
Copy link
Contributor

alexander-rukin commented Feb 2, 2022

It can act like this:

  • you show tooltip when user sees it
  • if he hovers it (or other icon) it's hidden for this "app run"
  • if user clicks one of those icons it's marked as "seen" and never shown automatically again

cc @daniloprates

@alexander-rukin
Copy link
Contributor

image

z index should be changed + scroll is not smoth 🤷

@alexander-rukin
Copy link
Contributor

image

position is a bit wrong -> we use 4px distance from element for tooltip positioning, and in this case element we are pointing to is icon, not container - reference https://zpl.io/VOEEv6r

@renanvalentin renanvalentin force-pushed the feature/ddw-852-stake-pools-list branch from bb4f3ca to 0f42f7e Compare February 2, 2022 20:02
@renanvalentin renanvalentin removed the WIP label Feb 2, 2022
@renanvalentin
Copy link
Contributor Author

image

z index should be changed + scroll is not smoth 🤷

@alexander-rukin created a new issue to fix the zIndex problem as it will result in regression testing:
https://input-output.atlassian.net/browse/DDW-939

@alexander-rukin
Copy link
Contributor

alexander-rukin commented Feb 9, 2022

image

hover issue is still not fixed

Copy link

@miorsufianiohk miorsufianiohk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Great work @renanvalentin . Tested on 20763 👍

@renanvalentin renanvalentin force-pushed the feature/ddw-852-stake-pools-list branch from 33d1357 to f0ec96e Compare February 9, 2022 20:01
@renanvalentin
Copy link
Contributor Author

image

hover issue is still not fixed

@alexander-rukin should've been fixed now

Screen Shot 2022-02-09 at 16 59 47

@alexander-rukin
Copy link
Contributor

alexander-rukin commented Feb 10, 2022

#2847 (comment)

hm, this hover effect doesn't look right 😓 it should just go from 0.3 to 0.7 current problem is probably because of "< button >" usage

current hover is 1 -> should be 0.7

image

@renanvalentin
Copy link
Contributor Author

@alexander-rukin done 👏

@alexander-rukin
Copy link
Contributor

screencast.2022-02-10.18-38-53.mp4

I've noticed strange behaviour =/
sorry 🙏

Copy link
Contributor

@alexander-rukin alexander-rukin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@danielmain danielmain merged commit 2a37312 into develop Feb 14, 2022
@danielmain danielmain deleted the feature/ddw-852-stake-pools-list branch February 14, 2022 17:10
renanvalentin added a commit that referenced this pull request Mar 7, 2022
Co-authored-by: Daniel Main <daniel.main.cernhoff@icloud.com>
renanvalentin added a commit that referenced this pull request Mar 7, 2022
Co-authored-by: Daniel Main <daniel.main.cernhoff@icloud.com>
renanvalentin added a commit that referenced this pull request Mar 7, 2022
Co-authored-by: Daniel Main <daniel.main.cernhoff@icloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants