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.

Sign up for GitHub

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

Jump to bottom

[DDW-1017] Fix margin for dialogs content and token table header #2944

Merged
merged 9 commits into from Apr 12, 2022
Merged

[DDW-1017] Fix margin for dialogs content and token table header #2944

merged 9 commits into from Apr 12, 2022

Conversation

lucas-barros
Copy link
Contributor

@lucas-barros lucas-barros commented Apr 1, 2022 •
edited

This PR fixes margins for dialogs content and token table header. Most changes were to ensure the dialogs followed this specification.

Affected components:

  • Delagation wizard dialog
  • Wallet restore dialog
  • Wallet receive dialog
  • Wallet import

Added two storybook flags to show buttons on receive screen dialog.

Story: Receive - sequential with address verification
Flags: isAddressDerived and isAddressChecked

Todos

  • TODO

Screenshots

Before After
image image
image image
image image

Testing Checklist

Review Checklist

Basics

  • PR assigned to the PR author(s)
  • input-output-hk/daedalus-dev and input-output-hk/daedalus-qa assigned as PR reviewers
  • If there are UI changes, Alexander Rukin assigned as an additional reviewer
  • All visual regression testing has been reviewed (assign run Chromatic label to PR to trigger the run)
  • PR has appropriate labels (release-vNext, feature/bug/chore, WIP)
  • PR link is added to a Jira ticket, ticket moved to In Review
  • PR is updated to the most recent version of the target branch (and there are no conflicts)
  • PR has a good description that summarizes all changes
  • PR contains screenshots (in case of UI changes)
  • CHANGELOG entry has been added to the top of the appropriate section (Features, Fixes, Chores) and is linked to the correct PR on GitHub
  • There are no missing translations (running yarn manage:translations produces no changes)
  • Text changes are proofread and approved (Jane Wild / Amy Reeve)
  • Japanese text changes are proofread and approved (Junko Oda)
  • Storybook works and no stories are broken (yarn storybook)
  • In case of dependency changes yarn.lock file is updated

Code Quality

  • Important parts of the code are properly commented and documented
  • Code is properly typed with typescript types
  • React components are split-up enough to avoid unnecessary re-renderings
  • Any code that only works in main process is neatly separated from components

Testing

  • New feature/change is covered by acceptance tests
  • New feature/change is manually tested and approved by QA team
  • All existing acceptance tests are still up-to-date
  • New feature/change is covered by Daedalus Testing scenario
  • All existing Daedalus Testing scenarios are still up-to-date

After Review

  • Update Slack QA thread by marking it with a green checkmark

@lucas-barros lucas-barros self-assigned this Apr 1, 2022
@lucas-barros lucas-barros marked this pull request as ready for review April 1, 2022 12:53
alexander-rukin
alexander-rukin suggested changes Apr 4, 2022
View reviewed changes
source/renderer/app/components/widgets/Dialog.scss Outdated
@@ -53,6 +53,10 @@
flex-shrink: 0;
margin-top: 10px;

&:not(:empty) {
margin-top: 30px;
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't solve problem

Copy link
Contributor

Choose a reason for hiding this comment

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

image

now, with expansion of bottom area scroll end position is off. It should be at the same level like the end of input field

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 fixed the scroll position. As for the change in the margin, there is no more margin bottom on the dialog content, so the scroll will appear when necessary without take in account the margin bottom that was removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

does it match this https://zpl.io/VkABvQg ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should match now.

@lucas-barros lucas-barros force-pushed the fix/ddw-1017-fix-bottom-margin-for-dialogs-content branch from 3cb257c to 1c170c0 Compare April 4, 2022 13:52
@alexander-rukin
Copy link
Contributor

image

scroll ends in incorrect position here

@alexander-rukin
Copy link
Contributor

alexander-rukin commented Apr 4, 2022 •
edited

image

now bottom fixed area is 20px more than before https://zpl.io/VkABvQg

@lucas-barros
Copy link
Contributor Author

image

scroll ends in incorrect position here

This is due to margin on the radio set component, not scrollbar. I can fix that also.
Screenshot from 2022-04-04 11-48-23

@lucas-barros lucas-barros force-pushed the fix/ddw-1017-fix-bottom-margin-for-dialogs-content branch from 1c170c0 to 455a348 Compare April 4, 2022 18:39
@lucas-barros
Copy link
Contributor Author

image

now bottom fixed area is 20px more than before https://zpl.io/VkABvQg

image

now bottom fixed area is 20px more than before https://zpl.io/VkABvQg

Updated.

@alexander-rukin
Copy link
Contributor

alexander-rukin commented Apr 5, 2022 •
edited

image

em... but now it's too small.

Here how it was:

  • before buttons you have 30px distance
  • it was 20px part of top scrollable content
  • 10px part of bottom fixed area

That's why it was not so trivial.

@lucas-barros lucas-barros force-pushed the fix/ddw-1017-fix-bottom-margin-for-dialogs-content branch from 455a348 to 74b067d Compare April 5, 2022 12:52
@lucas-barros
Copy link
Contributor Author

image

em... but now it's too small.

Here how it was:

  • before buttons you have 30px distance
  • it was 20px part of top scrollable content
  • 10px part of bottom fixed area

That's why it was not so trivial.

Ok, lets try again, I updated.

@alexander-rukin
Copy link
Contributor

alexander-rukin commented Apr 5, 2022 •
edited

image

3rd case scroll starting position is a bit off (but this one is not sooo critical, if we won't find more, we can probably ignore this bit) https://zpl.io/VkABvQg

UPD: on the next step position is OK

image

UPD2: on the third it's off again

image

@lucas-barros
Copy link
Contributor Author

image

3rd case scroll starting position is a bit off (but this one is not sooo critical, if we won't find more, we can probably ignore this bit) https://zpl.io/VkABvQg

UPD: on the next step position is OK

image

UPD2: on the third it's off again

image

Ok, fixed.

alexander-rukin
alexander-rukin approved these changes Apr 5, 2022
View reviewed changes
Copy link
Contributor

@alexander-rukin alexander-rukin left a comment

Choose a reason for hiding this comment

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

Now it looks great 👍

@lucas-barros lucas-barros removed the WIP label Apr 5, 2022
@lucas-barros lucas-barros requested review from a team April 5, 2022 16:53
@miorsufianiohk miorsufianiohk self-requested a review April 7, 2022 14:22
@lucas-barros lucas-barros force-pushed the fix/ddw-1017-fix-bottom-margin-for-dialogs-content branch from b27ee4e to 8bc63c7 Compare April 7, 2022 14:56
renanvalentin
renanvalentin approved these changes Apr 11, 2022
View reviewed changes
Copy link
Contributor

@renanvalentin renanvalentin left a comment

Choose a reason for hiding this comment

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

Great job @lucas-barros

ManusMcCole
ManusMcCole approved these changes Apr 11, 2022
View reviewed changes
Copy link

@ManusMcCole ManusMcCole left a comment

Choose a reason for hiding this comment

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

Looks good @lucas-barros 👍

danielmain
danielmain approved these changes Apr 12, 2022
View reviewed changes
Copy link
Contributor

@danielmain danielmain left a comment

Choose a reason for hiding this comment

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

Great job @lucas-barros

@danielmain danielmain merged commit 76cf83d into develop Apr 12, 2022
@danielmain danielmain deleted the fix/ddw-1017-fix-bottom-margin-for-dialogs-content branch April 12, 2022 15:29
lucas-barros pushed a commit that referenced this pull request Apr 25, 2022
@danielmain @lucas-barros
Merge pull request #2944 from input-output-hk/fix/ddw-1017-fix-bottom…
4839fe0 
…-margin-for-dialogs-content
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexander-rukin alexander-rukin alexander-rukin approved these changes

@renanvalentin renanvalentin renanvalentin approved these changes

@danielmain danielmain danielmain approved these changes

@ManusMcCole ManusMcCole ManusMcCole approved these changes

@miorsufianiohk miorsufianiohk Awaiting requested review from miorsufianiohk

Assignees

@lucas-barros lucas-barros

Labels
bug release-4.10.0 run Chromatic
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@lucas-barros @alexander-rukin @renanvalentin @danielmain @ManusMcCole