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

Refactor tooltip links (#3743) #3775

Merged
merged 8 commits into from
Nov 30, 2022

Conversation

traumschule
Copy link
Collaborator

@traumschule traumschule commented Oct 30, 2022

Fixes #3743
Except ExecutionRequirementsWarning.tsx where the link is part of a modal.

There are more in FundingDetailsStep.tsx. Are Fragments a better option than collating multiple paragraphs into one string (loosing newlines)?

For further refactoring texts can be pulled into dictionaries (#1989).

@vercel
Copy link

vercel bot commented Oct 30, 2022

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

Name Status Preview Updated
dao ✅ Ready (Inspect) Visit Preview Nov 25, 2022 at 8:52AM (UTC)
pioneer-2 ✅ Ready (Inspect) Visit Preview Nov 25, 2022 at 8:52AM (UTC)
pioneer-2-storybook ✅ Ready (Inspect) Visit Preview Nov 25, 2022 at 8:52AM (UTC)

Copy link
Collaborator

@thesan thesan left a comment

Choose a reason for hiding this comment

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

Nice refactor! Thanks

Copy link
Collaborator

@thesan thesan left a comment

Choose a reason for hiding this comment

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

Actually the new link text and icon are a bit harder to spot now

Old one:
Screenshot from 2022-11-03 16-18-49
New One:
Screenshot from 2022-11-03 16-18-40

Please fix the link font size and logo color

@traumschule
Copy link
Collaborator Author

@oleksanderkorn the last commit broke types, can you please also see if there's an easy fix for the CSS. For example:

ERROR in src/proposals/modals/AddNewProposal/components/StakingAccountStep.tsx:39:6
TS2322: Type 'string' is not assignable to type 'ReactElement<any, string | JSXElementConstructor<any>> | undefined'.
    37 |             tooltipText="The budget is the root resource pool for all token minting in the working group, and the size of the pool is denoted by budget."
    38 |             tooltipLinkURL="https://joystream.gitbook.io/joystream-handbook/key-concepts/staking#locks-1"
  > 39 |      tooltipLinkText="Learn more"
       |      ^^^^^^^^^^^^^^^

Copy link
Contributor

@oleksanderkorn oleksanderkorn left a comment

Choose a reason for hiding this comment

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

@traumschule
Fixing type is simple:

tooltipLinkText?: React.ReactElement | string

image

Also, there are unused imports:
image

@oleksanderkorn
Copy link
Contributor

oleksanderkorn commented Nov 7, 2022

@traumschule link was before inside the Tooltip Text, when I put it back the link name looks good.
image

For link to pick up the styles i had to add those two classes (i have no idea how it was working before)
image

@oleksanderkorn
Copy link
Contributor

oleksanderkorn commented Nov 7, 2022

@traumschule here is the full patch:

index c5ffc06b..d055af7b 100644
--- a/packages/ui/src/accounts/components/StakeStep.tsx
+++ b/packages/ui/src/accounts/components/StakeStep.tsx
@@ -8,10 +8,8 @@ import { Account, LockType } from '@/accounts/types'
 import { CurrencyName } from '@/app/constants/currency'
 import { InputComponent, TokenInput } from '@/common/components/forms'
 import { getErrorMessage, hasError } from '@/common/components/forms/FieldError'
-import { LinkSymbol } from '@/common/components/icons/symbols'
 import { Row } from '@/common/components/Modal'
 import { RowGapBlock } from '@/common/components/page/PageContent'
-import { TooltipExternalLink } from '@/common/components/Tooltip'
 import { TextMedium, TokenValue } from '@/common/components/typography'
 import { formatTokenValue } from '@/common/model/formatters'
 import { VoteForCouncilEvent, VoteForCouncilMachineState } from '@/council/modals/VoteForCouncil/machine'
diff --git a/packages/ui/src/app/pages/Election/Election.tsx b/packages/ui/src/app/pages/Election/Election.tsx
index cc2696e2..2d8be6f1 100644
--- a/packages/ui/src/app/pages/Election/Election.tsx
+++ b/packages/ui/src/app/pages/Election/Election.tsx
@@ -4,12 +4,11 @@ import { useHistory } from 'react-router-dom'
 import { PageHeaderRow, PageHeaderWrapper, PageLayout } from '@/app/components/PageLayout'
 import { ButtonsGroup, CopyButtonTemplate } from '@/common/components/buttons'
 import { LinkIcon } from '@/common/components/icons'
-import { LinkSymbol } from '@/common/components/icons/symbols'
 import { Loading } from '@/common/components/Loading'
 import { MainPanel } from '@/common/components/page/PageContent'
 import { PageTitle } from '@/common/components/page/PageTitle'
 import { BlockDurationStatistics, StatisticItem, Statistics } from '@/common/components/statistics'
-import { TextHuge, TextMedium } from '@/common/components/typography'
+import { TextHuge } from '@/common/components/typography'
 import { camelCaseToText } from '@/common/helpers'
 import { useRefetchQueries } from '@/common/hooks/useRefetchQueries'
 import { MILLISECONDS_PER_BLOCK } from '@/common/model/formatters'
diff --git a/packages/ui/src/bounty/modals/ContributeFundsModal/ContributeFundsModal.tsx b/packages/ui/src/bounty/modals/ContributeFundsModal/ContributeFundsModal.tsx
index 85ef0d27..3910c24e 100644
--- a/packages/ui/src/bounty/modals/ContributeFundsModal/ContributeFundsModal.tsx
+++ b/packages/ui/src/bounty/modals/ContributeFundsModal/ContributeFundsModal.tsx
@@ -20,7 +20,6 @@ import { SuccessTransactionModal } from '@/bounty/modals/SuccessTransactionModal
 import { isFundingLimited } from '@/bounty/types/Bounty'
 import { Input, InputComponent, TokenInput } from '@/common/components/forms'
 import { getErrorMessage, hasError } from '@/common/components/forms/FieldError'
-import { LinkSymbol } from '@/common/components/icons/symbols'
 import {
   AmountButton,
   AmountButtons,
diff --git a/packages/ui/src/common/components/Tooltip/Tooltip.tsx b/packages/ui/src/common/components/Tooltip/Tooltip.tsx
index d170591b..f5f1a753 100644
--- a/packages/ui/src/common/components/Tooltip/Tooltip.tsx
+++ b/packages/ui/src/common/components/Tooltip/Tooltip.tsx
@@ -4,11 +4,12 @@ import { usePopper } from 'react-popper'
 import { Link } from 'react-router-dom'
 import styled from 'styled-components'
 
+import { TextMedium } from '@/common/components/typography'
+
 import { BorderRad, Colors, Fonts, Transitions, ZIndex } from '../../constants'
 import { LinkSymbol, LinkSymbolStyle } from '../icons/symbols'
 
 import { DefaultTooltip } from './TooltipDefault'
-import { TextMedium } from '@/common/components/typography'
 
 export interface TooltipProps extends Omit<TooltipPopupProps, 'popUpHandlers' | 'position'> {
   absolute?: boolean
@@ -130,17 +131,21 @@ export const Tooltip = ({
                 forBig={forBig}
               >
                 {tooltipTitle && <TooltipPopupTitle>{tooltipTitle}</TooltipPopupTitle>}
-                <TooltipText>{tooltipText}</TooltipText>
-                {tooltipLinkURL &&
-                  (isExternalLink() ? (
-                    <TooltipExternalLink href={tooltipLinkURL} target="_blank">
-                      <TextMedium>{tooltipLinkText ?? 'Link'}</TextMedium> <LinkSymbol />
-                    </TooltipExternalLink>
-                  ) : (
-                    <TooltipLink to={tooltipLinkURL} target="_blank">
-                      <TextMedium>{tooltipLinkText ?? 'Link'}</TextMedium> <LinkSymbol />
-                    </TooltipLink>
-                  ))}
+                <TooltipText>
+                  {tooltipText}
+                  {tooltipLinkURL &&
+                    (isExternalLink() ? (
+                      <TooltipExternalLink href={tooltipLinkURL} target="_blank">
+                        <TextMedium>{tooltipLinkText ?? 'Link'}</TextMedium>
+                        <LinkSymbol />
+                      </TooltipExternalLink>
+                    ) : (
+                      <TooltipLink to={tooltipLinkURL} target="_blank">
+                        <TextMedium>{tooltipLinkText ?? 'Link'}</TextMedium>
+                        <LinkSymbol />
+                      </TooltipLink>
+                    ))}
+                </TooltipText>
               </TooltipPopupContainer>,
               document.body
             ))}
diff --git a/packages/ui/src/common/components/forms/InputComponent.tsx b/packages/ui/src/common/components/forms/InputComponent.tsx
index 51dc3dbd..ee2311d7 100644
--- a/packages/ui/src/common/components/forms/InputComponent.tsx
+++ b/packages/ui/src/common/components/forms/InputComponent.tsx
@@ -28,7 +28,7 @@ export type InputComponentProps = InputProps &
     message?: React.ReactElement | string
     tooltipText?: React.ReactElement | string
     tooltipTitle?: string
-    tooltipLinkText?: React.ReactElement
+    tooltipLinkText?: React.ReactElement | string
     tooltipLinkURL?: string
     className?: string
     children: React.ReactNode
diff --git a/packages/ui/src/common/components/icons/symbols/LinkSymbol.tsx b/packages/ui/src/common/components/icons/symbols/LinkSymbol.tsx
index 69b2a26c..8180ed14 100644
--- a/packages/ui/src/common/components/icons/symbols/LinkSymbol.tsx
+++ b/packages/ui/src/common/components/icons/symbols/LinkSymbol.tsx
@@ -15,10 +15,12 @@ export function LinkSymbol({ className, color }: SymbolProps) {
       className={className}
     >
       <path
+        className="blackPart"
         d="M1.1001 3.99998L2.0001 3.09998H12.8001V4.89998H2.9001V21.1H19.1001V11.2H20.9001V22L20.0001 22.9H2.0001L1.1001 22V3.99998Z"
         fill={color ?? Colors.Black[900]}
       />
       <path
+        className="primaryPart"
         d="M15.4999 1.09998H21.9999L22.8999 1.99998V8.49998H21.0999V4.17277L10.6363 14.6364L9.36353 13.3636L19.8271 2.89998H15.4999V1.09998Z"
         fill={color ?? Colors.Blue[500]}
       />
diff --git a/packages/ui/src/memberships/modals/BuyMembershipModal/BuyMembershipFormModal.tsx b/packages/ui/src/memberships/modals/BuyMembershipModal/BuyMembershipFormModal.tsx
index 84f655cf..d7ecfeba 100644
--- a/packages/ui/src/memberships/modals/BuyMembershipModal/BuyMembershipFormModal.tsx
+++ b/packages/ui/src/memberships/modals/BuyMembershipModal/BuyMembershipFormModal.tsx
@@ -21,7 +21,6 @@ import {
   ToggleCheckbox,
 } from '@/common/components/forms'
 import { Arrow } from '@/common/components/icons'
-import { LinkSymbol } from '@/common/components/icons/symbols'
 import { Loading } from '@/common/components/Loading'
 import {
   ModalFooter,
@@ -33,7 +32,6 @@ import {
   ScrolledModalContainer,
   TransactionInfoContainer,
 } from '@/common/components/Modal'
-import { TooltipExternalLink } from '@/common/components/Tooltip'
 import { TransactionInfo } from '@/common/components/TransactionInfo'
 import { TextMedium } from '@/common/components/typography'
 import { definedValues } from '@/common/utils'
diff --git a/packages/ui/src/memberships/modals/InviteMemberModal/InviteMemberFormModal.tsx b/packages/ui/src/memberships/modals/InviteMemberModal/InviteMemberFormModal.tsx
index ee6eabd6..32390dfe 100644
--- a/packages/ui/src/memberships/modals/InviteMemberModal/InviteMemberFormModal.tsx
+++ b/packages/ui/src/memberships/modals/InviteMemberModal/InviteMemberFormModal.tsx
@@ -4,7 +4,6 @@ import * as Yup from 'yup'
 
 import { ButtonPrimary } from '@/common/components/buttons'
 import { InputComponent, InputText, InputTextarea } from '@/common/components/forms'
-import { LinkSymbol } from '@/common/components/icons/symbols'
 import { Loading } from '@/common/components/Loading'
 import {
   ModalFooter,
@@ -14,7 +13,6 @@ import {
   ScrolledModalContainer,
   Row,
 } from '@/common/components/Modal'
-import { TooltipExternalLink } from '@/common/components/Tooltip'
 import { TextMedium } from '@/common/components/typography'
 import { useKeyring } from '@/common/hooks/useKeyring'
 import { useYupValidationResolver } from '@/common/utils/validation'
diff --git a/packages/ui/src/proposals/components/ProposalList/ProposalListItem.tsx b/packages/ui/src/proposals/components/ProposalList/ProposalListItem.tsx
index 6e04d27f..f311c8eb 100644
--- a/packages/ui/src/proposals/components/ProposalList/ProposalListItem.tsx
+++ b/packages/ui/src/proposals/components/ProposalList/ProposalListItem.tsx
@@ -5,12 +5,11 @@ import styled from 'styled-components'
 import { BadgeStatus } from '@/common/components/BadgeStatus'
 import { CopyButtonTemplate } from '@/common/components/buttons'
 import { LinkIcon } from '@/common/components/icons'
-import { LinkSymbol } from '@/common/components/icons/symbols'
 import { TableListItem } from '@/common/components/List'
 import { GhostRouterLink } from '@/common/components/RouterLink'
 import { Tooltip, TooltipDefault } from '@/common/components/Tooltip'
 import { Subscription } from '@/common/components/typography/Subscription'
-import { TextSmall, TextMedium } from '@/common/components/typography/Text'
+import { TextSmall } from '@/common/components/typography/Text'
 import { Colors, Overflow } from '@/common/constants'
 import { camelCaseToText } from '@/common/helpers'
 import { toDDMMYY } from '@/common/utils/dates'
diff --git a/packages/ui/src/proposals/modals/AddNewProposal/components/SpecificParameters/SetReferralCut.tsx b/packages/ui/src/proposals/modals/AddNewProposal/components/SpecificParameters/SetReferralCut.tsx
index 7ed4e0d8..37e2b793 100644
--- a/packages/ui/src/proposals/modals/AddNewProposal/components/SpecificParameters/SetReferralCut.tsx
+++ b/packages/ui/src/proposals/modals/AddNewProposal/components/SpecificParameters/SetReferralCut.tsx
@@ -2,10 +2,8 @@ import React from 'react'
 
 import { useApi } from '@/api/hooks/useApi'
 import { InputComponent, InputNumber } from '@/common/components/forms'
-import { LinkSymbol } from '@/common/components/icons/symbols'
 import { Row } from '@/common/components/Modal'
 import { RowGapBlock } from '@/common/components/page/PageContent'
-import { TooltipExternalLink } from '@/common/components/Tooltip'
 import { TextMedium, TokenValue } from '@/common/components/typography'
 import { useFirstObservableValue } from '@/common/hooks/useFirstObservableValue'
 
diff --git a/packages/ui/src/proposals/modals/AddNewProposal/components/StakingAccountStep.tsx b/packages/ui/src/proposals/modals/AddNewProposal/components/StakingAccountStep.tsx
index 8e98ae9d..68f86447 100644
--- a/packages/ui/src/proposals/modals/AddNewProposal/components/StakingAccountStep.tsx
+++ b/packages/ui/src/proposals/modals/AddNewProposal/components/StakingAccountStep.tsx
@@ -3,10 +3,8 @@ import React from 'react'
 
 import { SelectStakingAccount } from '@/accounts/components/SelectAccount'
 import { InputComponent } from '@/common/components/forms'
-import { LinkSymbol } from '@/common/components/icons/symbols'
 import { Row } from '@/common/components/Modal'
 import { RowGapBlock } from '@/common/components/page/PageContent'
-import { TooltipExternalLink } from '@/common/components/Tooltip'
 import { TextMedium, TokenValue } from '@/common/components/typography'
 
 interface StakingAccountStepProps {
@@ -36,7 +34,7 @@ export const StakingAccountStep = ({ requiredStake }: StakingAccountStepProps) =
             label="Select account for Staking"
             tooltipText="The budget is the root resource pool for all token minting in the working group, and the size of the pool is denoted by budget."
             tooltipLinkURL="https://joystream.gitbook.io/joystream-handbook/key-concepts/staking#locks-1"
-	    tooltipLinkText="Learn more"
+            tooltipLinkText="Learn more"
             inputSize="l"
             required
             name="stakingAccount.stakingAccount"

@oleksanderkorn
Copy link
Contributor

@traumschule https://github.com/traumschule/pioneer/pull/3/files

@traumschule
Copy link
Collaborator Author

Rebased and fixed test warnings which caused other conflicts, reverted and leaving this to ongoing tests refactory.

Copy link
Contributor

@oleksanderkorn oleksanderkorn left a comment

Choose a reason for hiding this comment

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

lgtm

)
return <PageLayout lastBreadcrumb={id} main={Main} />
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of this? Without useMemo on Main I don't see a reason

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reverted, even worse with useMemo

    const Main = useMemo(
      () => (
        (
          <RowGapBlock gap={24}>
            <ContentWithSidePanel>
              <Loading />
            </ContentWithSidePanel>
          </RowGapBlock>
        ),
        []
      )
    )
    return <PageLayout lastBreadcrumb={id} main={Main} />

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it worse? Extracting Main to the new variable doesn't have any positive impact either on performance or code clarity. Going in-depth - this block of HTML is so small that extracting it to the new variable doesn't really improve code clarity, that's why I said that the only viable reason would be to keep it in useMemo to process it once and serve when needed, but in this case, this if block will be executed only once, so useMemo doesn't make much sense as well.

Summarizing there was no need to change this code at all

Copy link
Collaborator Author

@traumschule traumschule Nov 25, 2022

Choose a reason for hiding this comment

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

Worse as in more lines of code is what i meant: i tried useMemo and the code was longer = even worse, the opposite of my motivation to change it in the first place). Thanks for clarifying, totally agree.

@traumschule
Copy link
Collaborator Author

Again sorry for the mess mixing in unrelated fixes. The failing test is about forum changes, not touching that as well.

Copy link
Contributor

@WRadoslaw WRadoslaw left a comment

Choose a reason for hiding this comment

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

Nice job!

@thesan thesan changed the base branch from dev to staging November 30, 2022 15:33
@thesan thesan merged commit 94effcc into Joystream:staging Nov 30, 2022
XxFlameCatxX pushed a commit to XxFlameCatxX/pioneer that referenced this pull request Dec 27, 2022
* Refactor tooltip links (Joystream#3743)

* Add tooltipLinkText, default TooltipExternalLink formatting

* PR Review fixes.

* fix import

* remove unused, mixed tabs and spaces

* Fix handbook links revised (Joystream#2847)

* Fix proposals trigger tooltipText (Joystream#2346)

* style

Co-authored-by: Joystream Stats <dev@joystreamstats.live>
Co-authored-by: Oleksandr Korniienko <oleksanderkorn@gmail.com>
XxFlameCatxX pushed a commit to XxFlameCatxX/pioneer that referenced this pull request Jan 14, 2023
* Refactor tooltip links (Joystream#3743)

* Add tooltipLinkText, default TooltipExternalLink formatting

* PR Review fixes.

* fix import

* remove unused, mixed tabs and spaces

* Fix handbook links revised (Joystream#2847)

* Fix proposals trigger tooltipText (Joystream#2346)

* style

Co-authored-by: Joystream Stats <dev@joystreamstats.live>
Co-authored-by: Oleksandr Korniienko <oleksanderkorn@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-dev issue suitable for community-dev pipeline jsg-code-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor TooltipExternalLink
4 participants