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

Fixing back arrow alignment/display logic [FTX Widget] #8947

Merged
merged 1 commit into from
Jun 9, 2021

Conversation

ryanml
Copy link
Contributor

@ryanml ryanml commented May 27, 2021

Fixes brave/brave-browser#15941
Fixes brave/brave-browser#15930

Initial state:
Screen Shot 2021-05-26 at 10 50 07 PM

Opt-in state:
Screen Shot 2021-05-26 at 10 50 15 PM

After viewing markets (corrected alignment):
Screen Shot 2021-05-26 at 10 50 28 PM

After viewing markets and switching to another widget (no persistence):
Screen Shot 2021-05-26 at 10 50 43 PM

cc: @simonhong

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Defined in issue

@ryanml ryanml force-pushed the ftx-back-arrow-fixes branch from 1286e11 to c0606ab Compare May 27, 2021 05:56
@@ -278,7 +278,7 @@ export const ListItem = styled('li')<StyleProps>`
export const BackArrow = styled('div')<StyleProps>`
width: 20px;
cursor: pointer;
margin-left: ${p => p.marketView ? 60 : 0}px;
margin-left: ${p => p.marketView ? 137 : 0}px;
Copy link
Contributor Author

@ryanml ryanml May 27, 2021

Choose a reason for hiding this comment

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

It seems the only place this shared style is used is within the FTX widget. If this were to be a general style for the backarrow as it appears in only the header across widgets, styles may look something like this:

width: 20px;
cursor: pointer;
position: absolute;
right: 40px;

Given that this is also used in the asset detail view, to cover that use case as well, it may be best to nix all css rules besides width and cursor, and just put the burden of positioning on containers fitting the use case

const shouldShowBackArrow = !selectedAsset &&
this.props.ftx.currentView !== ViewType.OptIn &&
!this.props.ftx.isConnected
const shouldShowBackArrow = showContent && !selectedAsset &&
Copy link
Member

Choose a reason for hiding this comment

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

IMO, back arrow could be visible when current view is markets and in not connected state.
and I think back arrow would be helpful also when asset is selected like below.
With this user can go back to opt-in view easily. WDYT? @ryanml @petemill

    const shouldShowBackArrow = showContent &&
      this.props.ftx.currentView === ViewType.Markets &&
      !this.props.ftx.isConnected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@simonhong, makes sense to me! I've updated this with:

--- a/components/brave_new_tab_ui/widgets/ftx/components/index.tsx
+++ b/components/brave_new_tab_ui/widgets/ftx/components/index.tsx
@@ -119,10 +119,9 @@ class FTX extends React.PureComponent<Props, State> {
   }
 
   renderTitle () {
-    const selectedAsset = this.props.ftx.assetDetail?.currencyName
     const { showContent, widgetTitle } = this.props
     // Only show back arrow to go back to opt-in view
-    const shouldShowBackArrow = showContent && !selectedAsset &&
+    const shouldShowBackArrow = showContent &&
       this.props.ftx.currentView !== ViewType.OptIn

Though I omitted the check for !this.props.ftx.isConnected, as I believe we still want it shown in the connected state as well. Let me know if that isn't the case, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, or it could be that we don't want it to show when connected because at that point, the user should be disconnecting the widget as opposed to backtracking to pre-optin via the back arrow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've re-updated this with your suggested snippet 👍

@ryanml ryanml force-pushed the ftx-back-arrow-fixes branch from c0606ab to be345c4 Compare June 7, 2021 07:49
@ryanml ryanml force-pushed the ftx-back-arrow-fixes branch from be345c4 to 3205b11 Compare June 7, 2021 07:57
Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

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

++ 👍

@simonhong
Copy link
Member

CI run from #9054 has all green.

@simonhong simonhong merged commit 4c6f795 into brave:master Jun 9, 2021
@simonhong simonhong added this to the 1.27.x - Nightly milestone Jun 9, 2021
petemill pushed a commit that referenced this pull request Jun 14, 2021
Fixing back arrow alignment/display logic [FTX Widget]
@srirambv
Copy link
Contributor

Verification passed on

Brave 1.27.59 Chromium: 91.0.4472.101 (Official Build) nightly (x86_64)
Revision af52a90bf87030dd1523486a1cd3ae25c5d76c9b-refs/branch-heads/4472@{#1462}
OS macOS Version 10.15.7 (Build 19H114)

brave/brave-browser#15941

  • Verified back button is not shown when widget is backgrounded

brave/brave-browser#15930

  • Verified back button is properly aligned compared to other widgets
FTX.NTP.mov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Back button on FTX is always shown Back button is not properly aligned on FTX widget
3 participants