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

fix: download Button name and tooltip added #1441

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mariyaraj
Copy link

Fix for #1185

@ericleponner
Copy link
Collaborator

Please join a snapshot to your PR.

@mariyaraj
Copy link
Author

Please join a snapshot to your PR.

Could you clarify what kind of snapshot you need? I am new here and I am not sure, what should I do?

@ericleponner
Copy link
Collaborator

ericleponner commented Oct 18, 2024

Could you clarify what kind of snapshot you need? I am new here and I am not sure, what should I do?

#1185 includes two screenshots ; could you include the same screenshots with the visual changes you are proposing.

Copy link
Collaborator

@ericleponner ericleponner left a comment

Choose a reason for hiding this comment

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

#1185 includes two screenshots ; could you include the same screenshots with the visual changes you are proposing.

@mariyaraj
Copy link
Author

mariyaraj commented Oct 24, 2024

@ericleponner Here are screenshots after my changes:
ButtonDownloadTransactionHistory
UntertitleAdded

@ericleponner ericleponner self-requested a review October 25, 2024 10:16
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Download Transaction History is long : we should replace it by Download.
  • Download Transaction History is fine for title attribute but cannot be hard-coded because DownloadButton is re-used in other views and different contexts
    • so title / tooltip must be parameterized by adding a new prop to DownloadButton (eg buttonTitle)
    • some tooltip text must be defined for other DownloadButton usage ; your proposals are welcome
  • when window width is small, we should hide Downloadand keep the icon only (as it is today)
    • Footer view (line 40) shows how to do that

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is useful for record keeping and tax reporting. is a bit too much specific : remove it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

  • dialog titles are left-aligned everywhere so keep it left-aligned
  • inline help text should be left-aligned too (we have other dialogs like this)
  • remove This is useful …
  • when possible we try to avoid specific CSS definitions and prefer to use Bulma style classes
    • here you can probably achieve the same visual with has-text-grey and mt-x classes
    • and remove dialog-title-container and dialog-subtitle

@ericleponner ericleponner changed the title Download Button name and tooltip added fix: download Button name and tooltip added Oct 25, 2024
@mariyaraj
Copy link
Author

@ericleponner Changes committed. Here the screenshots:
ButtonDownloadTransactionHistory
DownloadButton
DownloadDialogScreen

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.

2 participants