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

Tooltip for deactivated AddProposalButton (#3909) #3916

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

traumschule
Copy link
Contributor

@traumschule traumschule commented Dec 2, 2022

#3909

Wrap TransactionButton in a tooltip when disabled. The placement prop seemed necessary to better orient the arrow.

Note ideally we don't want to hardcode constants, however there is no quick solution with useProposalConstants because it is designed to query proposalsCodex not proposalsEngine.activeProposalCount and can't be easily extended without changing its return type.
Waiting for the go-ahead.

@vercel
Copy link

vercel bot commented Dec 2, 2022

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

Name Status Preview Comments Updated (UTC)
dao ❌ Failed (Inspect) 💬 Add feedback Jun 17, 2023 6:26am
pioneer-2 ❌ Failed (Inspect) 💬 Add feedback Jun 17, 2023 6:26am
pioneer-2-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 17, 2023 6:26am

Co-authored-by: Oleksandr Korniienko <oleksanderkorn@gmail.com>
@traumschule
Copy link
Contributor Author

before

FAIL test/proposals/components/AddProposalButton.test.tsx (8.094 s)
 |   ● UI: AddProposalButton › Active proposals under proposal limit
 | 
 |     expect(element).not.toBeDisabled()
 | 
 |     Received element is disabled:
 |       <button class="ButtonPrimaryStyles-sc-1s7nmh2-1 bOBZxM" disabled="" />
 | 
 |       17 |     renderComponent()
 |       18 |
 |     > 19 |     expect(await getButton('Add new proposal')).not.toBeDisabled()

after

 PASS  test/proposals/components/AddProposalButton.test.tsx
  UI: AddProposalButton
    ✓ Active proposals under proposal limit (109 ms)
    ✓ Active proposals limit met (17 ms)

Test Suites: 1 passed, 1 total
Tests:       2 passed, 2 total

Copy link
Member

@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.

@traumschule
Copy link
Contributor Author

Now the default is to show the general info with handbook link from the start even without connection or wallet. Clicking the button will open the modal to create a membership.
Should be fine because even new users will notice the Connect Wallet button at some point when asked to fill the fields.

proposals-tooltip


const tooltipProps = useMemo(() => {
if (!api?.isConnected) return { tooltipText: 'Connecting to api' }
if (!api?.isConnected || availableSlots > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (!api?.isConnected || availableSlots > 0) {
if (availableSlots > 0) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new logic is that until connection is established and when there are slots the default tooltip is shown. Without the first condition a user would see the error "Max active proposals limit reached" during loading.

Comment on lines 39 to 40
<Tooltip placement="bottom-end" {...tooltipProps}>
<TransactionButton style="primary" size="medium" onClick={addProposalModal} disabled={!availableSlots}>
Copy link
Member

Choose a reason for hiding this comment

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

TransactionButton also renders a Tooltip when a transaction is pending. As a result in case of a pending transaction, this will result in the Tooltip beeing nested (I'm not sure what will actually be shown). To avoid this situation, TransactionButton could accept TooltipContentProp:

  • If these are defined ⇒ display the Tooltip
  • If a transaction is pending ⇒ display the: "Please wait until the current transaction is over" regardless of whether a different tooltip text was passed.

WDYT ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes sense.

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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants