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.

Sign up for GitHub

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

Jump to bottom

[DDW-924] Create link component #1799

Merged
merged 28 commits into from
Jan 16, 2020
Merged

[DDW-924] Create link component #1799

merged 28 commits into from
Jan 16, 2020

Conversation

tomislavhoracek
Copy link
Contributor

@tomislavhoracek tomislavhoracek commented Dec 21, 2019 •
edited
Loading

This PR introduces React-Polymorph "Link" component integration.

Todos

  • Update react-polymorph to the latest version which includes Link component once is ready
  • Check components one more time and add RP Link component or ButtonLink widget if miss on some places
  • Add CHANGELOG once PR is ready

Screenshots

1 Nodes - Syncing and Connecting - System Time Error (NTP reachable)"
1 Nodes - Syncing and Connecting - System Time Error (NTP reachable)

1.1 Nodes - Syncing and Connecting - System Time Error"
1.1 Nodes - Syncing and Connecting - System Time Error

2 General - Support"
2 General - Support

3 Nodes - Splash - Network Info"
3 Nodes - Splash - Network Info

4 Decentralization - Staking - Rewards"
4 Decentralization - Staking - Rewards

5 Decentralization - Staking - Pool Index"
Screen Shot 2020-01-13 at 12 41 20

6 Nodes - About - About Dialog"
6 Nodes - About - About Dialog

7 Nodes - Status - Daedalus Diagnostics"
7 Nodes - Status - Daedalus Diagnostics

8 Wallets - Paper Wallets - Completion"
8 Wallets - Paper Wallets - Completion

9 Wallets - Paper Wallets - Instructions"
9 Wallets - Paper Wallets - Instructions

10 Wallets - Transactions"
10 Wallets - Transactions

11 Wallets - Transactions - UTXO Distribution"
11 Wallets - Transactions - UTXO Distribution

13 1 Nodes - Syncing and Connecting - Trouble Syncing"
13 1 Nodes - Syncing and Connecting - Trouble Syncing

13 2 Nodes - Syncing and Connecting - Trouble Connecting"
13 2 Nodes - Syncing and Connecting - Trouble Connecting

14 1 Nodes - Updates - Manual Update (Icon and label are inside button)"
14 1 Nodes - Updates - Manual Update (Icon and label are inside button)

14 2 Common - Widgets - ButtonLink"
14 2 Common - Widgets - ButtonLink

15 News - Alerts - Alerts Overlay (Icon and label are inside button)"
15 News - Alerts - Alerts Overlay (Icon and label are inside button)

16 News - Incident - Incident Overlay (Icon and label are inside button)"
16 News - Incident - Incident Overlay (Icon and label are inside button)

17 News - NewsFeed - Fetched (Icon and label are inside button)"
17 News - NewsFeed - Fetched (Icon and label are inside button)

18 Wallets - Summary - Wallet Summary + Legacy Wallet (Icon and label are inside button)"
18 Wallets - Summary - Wallet Summary + Legacy Wallet (Icon and label are inside button)

19 Decentralization - Countdown - Decentralization Countdown"
19 Decentralization - Countdown - Decentralization Countdown

20 Decentralization - Staking - Info"
20 Decentralization - Staking - Info

Testing Checklist

Storybook Testing:

1. LINKS WITH / WITHOUT ICON

  • Test Storybook stories under Nodes -> Errors -> System Time Error
  • Test Storybook stories under Settings -> General -> Support
  • Test Storybook stories under Nodes -> Splash -> Network Info
  • Test Storybook stories under Decentralization -> Staking -> Rewards
  • Test Storybook stories under Decentralization -> Staking -> Pool Index
  • Test Storybook stories under Nodes -> About -> About Dialog
  • Test Storybook stories under Nodes -> Status -> Daedalus Diagnostics (Available disk space: unknown)
  • Test Storybook stories under Wallets -> Paper Wallets -> Completion
  • Test Storybook stories under Wallets -> Paper Wallets -> Instructions
  • Test Storybook stories under Wallets -> Transactions
  • Test Storybook stories under Wallets -> Transactions -> UTXO Distribution
  • Test Storybook stories under Wallet Settings -> Wallet Recovery Phrase (Verification Failure Step Dialog)
  • Test Storybook stories under Nodes -> Syncing and Connecting -> Trouble Syncing
  • Test Storybook stories under Nodes -> Syncing and Connecting -> Trouble Connecting

2. BUTTON LINKS

  • Test Storybook stories under Nodes -> Updates -> Manual Update (Icon and label are inside button)
  • Test Storybook stories under Common -> Widgets -> ButtonLink
  • Test Storybook stories under News -> Alerts -> Alerts Overlay (Icon and label are inside button)
  • Test Storybook stories under News -> Incident -> Incident Overlay (Icon and label are inside button)
  • Test Storybook stories under News -> NewsFeed -> Fetched (Icon and label are inside button)
  • Test Storybook stories under Wallets -> Summary -> Wallet Summary + Legacy Wallet (Icon and label are inside button)
  • Test Storybook stories under Decentralization -> Countdown -> Decentralization Countdown
  • Test Storybook stories under Decentralization -> Staking -> Info

In-App Testing:

  • Follow storybook steps and test all in-app related screens. Pay attention that Link and ButtonLink clicks (open external / internal links) works properly

Review Checklist

Basics

  • PR has been assigned and has appropriate labels (feature/bug/chore, release-x.x.x)
  • 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 and shows some screenshots or animated GIFs of important 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
  • Automated tests: All acceptance and unit tests are passing (yarn test)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in development build (yarn dev)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in production build (yarn package / CI builds)
  • There are no flow errors or warnings (yarn flow:test)
  • There are no lint errors or warnings (yarn lint)
  • There are no prettier errors or warnings (yarn prettier:check)
  • 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)
  • UI changes look good in all themes (Alexander Rukin)
  • 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 flow
  • 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

  • Merge the PR
  • Delete the source branch
  • Move the ticket to done column on the YouTrack board
  • Update Slack QA thread by marking it with a green checkmark

Tomislav Horaček added 20 commits December 19, 2019 11:04
[DDW-924] Pull react-polymorph from GH repo and test
745182c
[DDW-924] Add Link overrides and integration into SystemTimeError com…
1c0b0dc 
…ponent
[DDW-924] Replace all links with icons with RP Link component
e80ba1e
[DDW-924] Replace link for WalletRecoveryPhraseStep4Dialog dialog and…
948fa81 
… small style improvements
[DDW-924] Remove RP Link test case and new anchor eslint ignore rule
3533c32
[DDW-924] Fix eslint issues
0da251c
[DDW-924] Fix Flow issues
92a187c
[DDW-924] Code cleanup
cf9c613
[DDW-924] Fix scss alphabetical ordering
6eb121a
[DDW-924] Add RP link theme variables
edd83b3
[DDW-924] Add RP Link for Donwload Logs syncing/connecting notification
2e9d704
[DDW-924] Introduce ButtonLink widget and imlementation into Manual U…
c8dca25 
…pdate overlay
[DDW-924] Add ButtonLink to Incident / Alert overlays
5413ec4
[DDW-924] Add ButtonLink to NewsFeed Item
6c7edbc
[DDW-924] Add ButtonLink to Legacy Notification
b466842
[DDW-924] Add ButtonLink to Staking Countdown
e683b94
[DDW-924] Add ButtonLink to Staking Info
75e8fb7
[DDW-924] Fix lint and Flow
b34dff8
[DDW-924] Fix yarn.lock
9448733
[DDW-924] Button link code improvement and cleanup
d8750cf
@tomislavhoracek tomislavhoracek self-assigned this Dec 21, 2019
@tomislavhoracek tomislavhoracek added ⏳release-vNext-ITN1 Daedalus Incentivized Testnet - Rewards DO NOT MERGE feature WIP labels Dec 21, 2019
@nikolaglumac
Copy link
Contributor

@MarcusHurney @tomothespian what's holding us up with this one?

@tomislavhoracek
Copy link
Contributor Author

@nikolaglumac now we have a new RP version with Link component. I will add a new package version and we are ready for review.

@ManusMcCole
Copy link

For 8 Wallets - Paper Wallets - Completion the text "WalletCertificateAddress" does not get translated as in the screenshot below
image

@nikolaglumac
Copy link
Contributor

@ManusMcCole that's expected... Please ignore this part.

@IuliaDolishniak
Copy link

2020-01-13_16-00-25
there is no 'Learn More' hyperlink on Rewards screen

@nikolaglumac
Copy link
Contributor

@IuliaDolishniak it has been removed on purpose. I did this part before I left for my vacation.

@IuliaDolishniak
Copy link

It is shown on screen and in Storybook @nikolaglumac

@nikolaglumac
Copy link
Contributor

@IuliaDolishniak screenshots are old. This link should not be visible neither in Daedalus nor Storybook.

@tomislavhoracek
Copy link
Contributor Author

@IuliaDolishniak there is no "Learn more" link for incentivized testnet.
You can see in storybook that we have Rewards and Rewards - ITN stories

@nikolaglumac
Copy link
Contributor

@tomothespian please merge latest v2-integration into this PR please!

@nikolaglumac
Copy link
Contributor

@DeeJayElly @MarcusHurney @yakovkaravelov please review this one!

DeeJayElly
DeeJayElly approved these changes Jan 14, 2020
View reviewed changes
Copy link
Contributor

@DeeJayElly DeeJayElly left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Good job @tomothespian !!!

Don't forgot to merge latest v2-integration

MarcusHurney
MarcusHurney suggested changes Jan 15, 2020
View reviewed changes
Copy link
Contributor

@MarcusHurney MarcusHurney left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@tomothespian The ButtonLink on the Alerts and and Incident Overlays is rendering the icon within the Link component above the button text. See screenshots below:

Screen Shot 2020-01-15 at 3 15 01 PM

Screen Shot 2020-01-15 at 3 15 12 PM

Update: I pushed a commit to fix both 😃

@MarcusHurney
Copy link
Contributor

Before we merge, let's get this question answered by Sasha.
@a-rukin Should the icon have a margin-right of 10px or 12px? It is currently set at 12px. I'm going to post some screenshots below showing the difference. I think 12px looks like too much!
(This is is on the decentralization countdown screen's button at the bottom)

12px
12px

10px
10px

@tomislavhoracek
Copy link
Contributor Author

I pushed a commit to fix both 😃

@MarcusHurney thanks a lot ❤️

@nikolaglumac nikolaglumac merged commit 5ea5c7e into v2-integration Jan 16, 2020
@iohk-bors iohk-bors bot deleted the feature/ddw-924-create-link-component branch January 16, 2020 08:33
@alexander-rukin
Copy link
Contributor

@MarcusHurney instead of tweaking 10px or 12px I'd unified all these buttons to be "text+icon" instead of both ("icon+text" or "text+icon").

topseniors pushed a commit that referenced this pull request Jan 19, 2020
@MarcusHurney @topseniors
[DDW-924] Create link component (#1799)
9abcb06 
* [DDW-924] Pull react-polymorph from GH repo and test

* [DDW-924] Add Link overrides and integration into SystemTimeError component

* [DDW-924] Replace all links with icons with RP Link component

* [DDW-924] Replace link for WalletRecoveryPhraseStep4Dialog dialog and small style improvements

* [DDW-924] Remove RP Link test case and new anchor eslint ignore rule

* [DDW-924] Fix eslint issues

* [DDW-924] Fix Flow issues

* [DDW-924] Code cleanup

* [DDW-924] Fix scss alphabetical ordering

* [DDW-924] Add RP link theme variables

* [DDW-924] Add RP Link for Donwload Logs syncing/connecting notification

* [DDW-924] Introduce ButtonLink widget and imlementation into Manual Update overlay

* [DDW-924] Add ButtonLink to Incident / Alert overlays

* [DDW-924] Add ButtonLink to NewsFeed Item

* [DDW-924] Add ButtonLink to Legacy Notification

* [DDW-924] Add ButtonLink to Staking Countdown

* [DDW-924] Add ButtonLink to Staking Info

* [DDW-924] Fix lint and Flow

* [DDW-924] Fix yarn.lock

* [DDW-924] Button link code improvement and cleanup

* [DDW-924] Introduce new React Polymorph version 0.9.1

* [DDW-924] Remove yarn.lock integrity hashes

* [DDW-924] CHANGELOG update

* [DDW-924] Fixes positioning of link icon in Alerts and Incident Overlays button

Co-authored-by: Marcus Hurney <marcushurney@gmail.com>
@nikolaglumac nikolaglumac added release-2.1.0-ITN1 Daedalus Incentivized Testnet - Rewards and removed ⏳release-vNext-ITN1 Daedalus Incentivized Testnet - Rewards labels Jan 22, 2020
@nikolaglumac nikolaglumac mentioned this pull request Apr 23, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ManusMcCole ManusMcCole ManusMcCole approved these changes

@IuliaDolishniak IuliaDolishniak IuliaDolishniak approved these changes

@DeeJayElly DeeJayElly DeeJayElly approved these changes

@MarcusHurney MarcusHurney MarcusHurney approved these changes

@thedanheller thedanheller Awaiting requested review from thedanheller

@topseniors topseniors Awaiting requested review from topseniors

Assignees

@tomislavhoracek tomislavhoracek

Labels
feature release-2.1.0-ITN1 Daedalus Incentivized Testnet - Rewards
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@tomislavhoracek @nikolaglumac @ManusMcCole @IuliaDolishniak @MarcusHurney @alexander-rukin @DeeJayElly