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

feat(account): loading state #748

Merged
merged 25 commits into from
Feb 11, 2019
Merged

feat(account): loading state #748

merged 25 commits into from
Feb 11, 2019

Conversation

mhuggins
Copy link
Contributor

@mhuggins mhuggins commented Dec 9, 2018

Description

This changes the way components and redux state are loaded in the account screen such that loading states can be rendered per panel within that screen. It still depends on some style updates from @fabric-8 before it will be ready.

The code changes here also will never transition from loading to loaded, simply because it's easier to observe the loading state during development. Before merging, that change must be reverted.

Motivation and Context

The current loading state is unappealing and doesn't match designs.

How Has This Been Tested?

Navigating to the account screen.

Screenshots (if appropriate)

screen shot 2018-12-08 at 9 02 55 pm

Types of changes

  • Chore (tests, refactors, and fixes)
  • New feature (adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have read the CONTRIBUTING guidelines and confirm that my code follows the code style of this project.
  • Tests for the changes have been added (for bug fixes/features)

Documentation

  • Docs need to be added/updated (for bug fixes/features)

Closing issues

Fixes #408

@mhuggins mhuggins added the PR: don't merge This doesn't seem right label Dec 9, 2018
@mhuggins mhuggins mentioned this pull request Dec 9, 2018
@DalderupMaurice
Copy link
Member

DalderupMaurice commented Dec 13, 2018

I know it's not ready yet but I'm throwing these in there already just so you know.

When loading fails:

Improvement Suggestion - the space seems a little small/tight and the address just barely fits in:

@DalderupMaurice
Copy link
Member

Merge conflicts :(

@DalderupMaurice DalderupMaurice added PR: needs review Pull request and removed PR: don't merge This doesn't seem right labels Jan 26, 2019
@DalderupMaurice
Copy link
Member

@fabric-8 could you have a sneak peak at the design side of things or guide me in the right direction?
The dimensions of the loading screen don't fit the actual loaded dimensions, which makes it jump when it's loaded.
Due to the chart's responsive layout which requires width: 0 and the fact that I cannot wrap it because it screws up the whole layout I don't really know how to make both states fit to create a smooth transition.

@fabric-8
Copy link
Contributor

fabric-8 commented Feb 7, 2019

@DalderupMaurice I was able to remove the flicker after doctoring around a lot :D However, I noticed that while the transaction panel has an entire "custom loading component/state" it should actually just appear to be disabled while loading is in progress: https://www.figma.com/file/apqKpVZZDzppAb0lVH69iiZn/nOS-Client?node-id=2755%3A38184 ... or did I miss something?

@codecov
Copy link

codecov bot commented Feb 9, 2019

Codecov Report

Merging #748 into develop will decrease coverage by 2.09%.
The diff coverage is 59.18%.

@@            Coverage Diff             @@
##           develop     #748     +/-   ##
==========================================
- Coverage    54.54%   52.45%   -2.1%     
==========================================
  Files          227      231      +4     
  Lines         1958     1977     +19     
  Branches       270      272      +2     
==========================================
- Hits          1068     1037     -31     
- Misses         733      776     +43     
- Partials       157      164      +7

@DalderupMaurice DalderupMaurice added PR: good to merge Reviewed and approved and removed PR: needs review Pull request labels Feb 9, 2019
@DalderupMaurice DalderupMaurice merged commit 88743ad into develop Feb 11, 2019
@DalderupMaurice DalderupMaurice deleted the feat/account-loading branch February 11, 2019 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: good to merge Reviewed and approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Account zero-state
3 participants