[LIVE-2943] Feat/add back forward buttons to web platform player #625

chabroA · 2022-07-11T16:13:39Z

📝 Description

Add the back and forward buttons to the web platform player, allowing to easily back and forth in a Live app and also between live apps (like with the Buy / Sell live-app that redirect to other live apps)

Possible to display or hide navigation through a new property in the TopBarConfig prop.

For now, only enables the navigation on the dedicated Ledger Card entry
The navigation on the Buy / Sell screen will be enabled by this PR -> #923

❓ Context

Impacted projects: ``
Linked resource(s): ``

✅ Checklist

Test coverage
Atomic delivery
No breaking changes

📸 Demo

b.mov

🚀 Expectations to reach

We can go back and forward on live apps.

Please make sure you follow these Important Steps.

Pull Requests must pass the CI and be internally validated in order to be merged.

vercel · 2022-07-11T16:13:43Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
live-common-tools	✅ Ready (Inspect)	Visit Preview	Aug 12, 2022 at 7:40AM (UTC)

3 Ignored Deployments

Name	Status	Updated
ledger-live-github-bot	⬜️ Ignored (Inspect)	Aug 12, 2022 at 7:40AM (UTC)
native-ui-storybook	⬜️ Ignored (Inspect)	Aug 12, 2022 at 7:40AM (UTC)
react-ui-storybook	⬜️ Ignored (Inspect)	Aug 12, 2022 at 7:40AM (UTC)

changeset-bot · 2022-07-11T16:13:43Z

🦋 Changeset detected

Latest commit: ae151d6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
ledger-live-desktop	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

codecov · 2022-07-11T16:38:28Z

Codecov Report

Merging #625 (2d1e7b1) into release (fe110c9) will increase coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 2d1e7b1 differs from pull request most recent head ae151d6. Consider uploading reports for the commit ae151d6 to get more accurate results

@@             Coverage Diff             @@
##           release     #625      +/-   ##
===========================================
+ Coverage    47.91%   47.92%   +0.01%     
===========================================
  Files          617      620       +3     
  Lines        27578    27870     +292     
  Branches      7126     7177      +51     
===========================================
+ Hits         13215    13358     +143     
- Misses       13296    13393      +97     
- Partials      1067     1119      +52

Flag	Coverage Δ
test	`47.92% <ø> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
libs/ledger-live-common/src/range.ts	`94.87% <0.00%> (-2.57%)`	⬇️
libs/ledgerjs/packages/hw-app-btc/src/BtcNew.ts	`92.25% <0.00%> (ø)`
libs/ledgerjs/packages/hw-app-btc/src/varint.ts	`30.76% <0.00%> (ø)`
.../ledgerjs/packages/hw-app-btc/src/newops/psbtv2.ts	`67.51% <0.00%> (ø)`
...dgerjs/packages/hw-app-btc/src/newops/appClient.ts	`23.07% <0.00%> (ø)`
...dgerjs/packages/hw-app-btc/src/newops/merkleMap.ts	`20.00% <0.00%> (ø)`
...dgerjs/packages/hw-app-btc/src/splitTransaction.ts	`74.22% <0.00%> (ø)`
...erjs/packages/hw-app-btc/src/newops/accounttype.ts	`65.78% <0.00%> (ø)`
...js/packages/hw-app-btc/src/newops/psbtFinalizer.ts	`78.57% <0.00%> (ø)`
...s/packages/hw-app-btc/src/newops/clientCommands.ts	`10.45% <0.00%> (ø)`
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

github-actions · 2022-07-11T16:43:44Z

@chabroA

Screenshots: ✅

There are no changes in the screenshots for this PR. If this is expected, you are good to go.

Justkant

LGTM just a question for the ref

apps/ledger-live-desktop/src/renderer/components/WebPlatformPlayer/index.jsx

Justkant · 2022-07-13T08:21:10Z

We also need a changeset for this PR

apps/ledger-live-desktop/src/renderer/components/WebPlatformPlayer/TopBar.jsx

gre · 2022-07-15T09:10:04Z

apps/ledger-live-desktop/src/renderer/components/WebPlatformPlayer/TopBar.jsx

  } = config;

+  const [canGoBack, setCanGoBack] = React.useState(false);
+  const [canGoForward, setCanGoForward] = React.useState(false);


it appears that canGoBack === canGoForward is kept true at any point in time, so you only need one state?

I need to keep track of both cases, to enable or disable the "forward" or "backward" button accordingly.

Furthermore, canGoBack === canGoForward is not true at all time. Sometimes you can go back but not forward, and some other times you can go forward but not back. And sometimes you can do both

apps/ledger-live-desktop/src/renderer/components/WebPlatformPlayer/index.jsx

gre

is it possible to have a unit test that instrument this to be able to test end to end?

We have to test all our PRs. and Desktop offers way to implement this. maybe a mock page could be used to record the events and assert it works? 🙏 would be super beneficial for non regression in future.

chabroA · 2022-08-05T13:50:12Z

is it possible to have a unit test that instrument this to be able to test end to end?

We have to test all our PRs. and Desktop offers way to implement this. maybe a mock page could be used to record the events and assert it works? 🙏 would be super beneficial for non regression in future.

After further investigation and tests, we currently don't have a proper testing framework to easily build and reliably maintain such tests.
IMHO we should consider working on such framework independently from this feature PR (it's also a broader R&D subject) rather than blocking it until such thing is available

cf. this PR as a non conclusive attempt to add such tests in the current framework

Justkant

small changes and we can merge this

apps/ledger-live-desktop/src/renderer/components/WebPlatformPlayer/TopBar.jsx

Justkant

LGTM

* add possibility to go back and forward * use arrow icon * move webview specific callbacks to TopBar * disable nav buttons when nav not possible * Add shouldDisplayNavigation to TopBarConfig * Use webview ref as prop instead of ref current value * add comments for events listened to * small code style changes * Add changeset

…yer (#625)" This reverts commit 3caf8e1.

* [LIVE-2943] Feat/add back forward buttons to web platform player (#625) * add possibility to go back and forward * use arrow icon * move webview specific callbacks to TopBar * disable nav buttons when nav not possible * Add shouldDisplayNavigation to TopBarConfig * Use webview ref as prop instead of ref current value * add comments for events listened to * small code style changes * Add changeset * use navigation is buy/sell screen * Revert "use navigation is buy/sell screen" This reverts commit 647b892. * handle Buy Sell live app in exchange screen * add changeset * Revert "[LIVE-2943] Feat/add back forward buttons to web platform player (#625)" This reverts commit 3caf8e1.

github-actions bot assigned chabroA Jul 11, 2022

github-actions bot added the desktop Has changes in LLD label Jul 11, 2022

chabroA force-pushed the feat/add-back-forward-buttons-web-platform-player branch from a039d3f to 62b87e1 Compare July 11, 2022 16:14

chabroA requested review from IAmMorrow and Justkant July 11, 2022 16:15

vercel bot deployed to Preview – live-common-tools July 11, 2022 16:15 View deployment

vercel bot deployed to Preview – live-common-tools July 12, 2022 07:14 View deployment

Justkant reviewed Jul 12, 2022

View reviewed changes

apps/ledger-live-desktop/src/renderer/components/WebPlatformPlayer/index.jsx Outdated Show resolved Hide resolved

gre suggested changes Jul 15, 2022

View reviewed changes

chabroA requested review from gre and Justkant July 19, 2022 08:58

vercel bot deployed to Preview – live-common-tools July 19, 2022 09:01 View deployment

gre reviewed Jul 19, 2022

View reviewed changes

gre self-assigned this Jul 26, 2022

chabroA force-pushed the feat/add-back-forward-buttons-web-platform-player branch from 78c0e1a to e4a223b Compare August 5, 2022 13:22

vercel bot deployed to Preview – live-common-tools August 5, 2022 13:23 View deployment

Justkant reviewed Aug 8, 2022

View reviewed changes

apps/ledger-live-desktop/src/renderer/components/WebPlatformPlayer/TopBar.jsx Outdated Show resolved Hide resolved

apps/ledger-live-desktop/src/renderer/components/WebPlatformPlayer/TopBar.jsx Outdated Show resolved Hide resolved

chabroA changed the title ~~Feat/add back forward buttons to web platform player~~ [LIVE-2943] Feat/add back forward buttons to web platform player Aug 10, 2022

chabroA requested review from Justkant and gre August 10, 2022 07:14

vercel bot deployed to Preview – live-common-tools August 10, 2022 07:14 View deployment

Justkant approved these changes Aug 10, 2022

View reviewed changes

chabroA force-pushed the feat/add-back-forward-buttons-web-platform-player branch from bb376a6 to b8e31a7 Compare August 10, 2022 08:42

vercel bot deployed to Preview – live-common-tools August 10, 2022 08:43 View deployment

vercel bot deployed to Preview – live-common-tools August 10, 2022 13:20 View deployment

chabroA changed the base branch from develop to release August 12, 2022 07:33

chabroA changed the base branch from release to develop August 12, 2022 07:33

chabroA changed the base branch from develop to release August 12, 2022 07:34

chabroA added 9 commits August 12, 2022 09:37

add possibility to go back and forward

5a5673c

use arrow icon

54ab819

move webview specific callbacks to TopBar

480c81a

disable nav buttons when nav not possible

792f2ad

Add shouldDisplayNavigation to TopBarConfig

75323d3

Use webview ref as prop instead of ref current value

72f7fea

add comments for events listened to

3153f5f

small code style changes

e15aded

Add changeset

ae151d6

chabroA force-pushed the feat/add-back-forward-buttons-web-platform-player branch from f940aab to ae151d6 Compare August 12, 2022 07:37

chabroA added the release candidate for next release label Aug 12, 2022

vercel bot deployed to Preview – live-common-tools August 12, 2022 07:38 View deployment

chabroA mentioned this pull request Aug 12, 2022

[LIVE-3341][FEATURE] use navigation for buy sell screen #923

Merged

3 tasks

chabroA merged commit 2589194 into release Aug 12, 2022

chabroA deleted the feat/add-back-forward-buttons-web-platform-player branch August 12, 2022 12:18

chabroA added a commit that referenced this pull request Aug 19, 2022

Revert "[LIVE-2943] Feat/add back forward buttons to web platform pla…

79712db

…yer (#625)" This reverts commit 3caf8e1.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LIVE-2943] Feat/add back forward buttons to web platform player #625

[LIVE-2943] Feat/add back forward buttons to web platform player #625

chabroA commented Jul 11, 2022 •

edited

Loading

vercel bot commented Jul 11, 2022 •

edited

Loading

changeset-bot bot commented Jul 11, 2022 •

edited

Loading

codecov bot commented Jul 11, 2022 •

edited

Loading

github-actions bot commented Jul 11, 2022 •

edited

Loading

Justkant left a comment

Justkant commented Jul 13, 2022

gre Jul 15, 2022

chabroA Jul 19, 2022

gre left a comment

chabroA commented Aug 5, 2022 •

edited

Loading

Justkant left a comment

Justkant left a comment

[LIVE-2943] Feat/add back forward buttons to web platform player #625

[LIVE-2943] Feat/add back forward buttons to web platform player #625

Conversation

chabroA commented Jul 11, 2022 • edited Loading

📝 Description

❓ Context

✅ Checklist

📸 Demo

🚀 Expectations to reach

vercel bot commented Jul 11, 2022 • edited Loading

changeset-bot bot commented Jul 11, 2022 • edited Loading

🦋 Changeset detected

codecov bot commented Jul 11, 2022 • edited Loading

Codecov Report

github-actions bot commented Jul 11, 2022 • edited Loading

Justkant left a comment

Choose a reason for hiding this comment

Justkant commented Jul 13, 2022

gre Jul 15, 2022

Choose a reason for hiding this comment

chabroA Jul 19, 2022

Choose a reason for hiding this comment

gre left a comment

Choose a reason for hiding this comment

chabroA commented Aug 5, 2022 • edited Loading

Justkant left a comment

Choose a reason for hiding this comment

Justkant left a comment

Choose a reason for hiding this comment

chabroA commented Jul 11, 2022 •

edited

Loading

vercel bot commented Jul 11, 2022 •

edited

Loading

changeset-bot bot commented Jul 11, 2022 •

edited

Loading

codecov bot commented Jul 11, 2022 •

edited

Loading

github-actions bot commented Jul 11, 2022 •

edited

Loading

chabroA commented Aug 5, 2022 •

edited

Loading