Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

fix: don't break the page structure while loading data for the headerbar #7

Merged
merged 3 commits into from
May 28, 2019

Conversation

amcgee
Copy link
Member

@amcgee amcgee commented May 27, 2019

Requires dhis2/app-runtime#11 to support loading states in Storybook.

Instead of rendering the disruptive and ugly ... paceholder while waiting for initial data to load, this keeps the macro structure of the headerbar in place but omits the data-dependent content. It will make page loads less jerky. We may want to indicate the loading state in some subtle way as well (@cooper-joe) but that can be a separate change.

Screenshot 2019-05-27 12 20 02

I also added a loading state story to the storybook.

@amcgee amcgee requested review from varl and cooper-joe May 27, 2019 10:22
const locale = data.user.settings.keyUiLocale || 'en'
i18n.changeLanguage(locale)
if (!loading) {
// TODO: This will run every render which is probably wrong! Also, setting the global locale shouldn't be done in the headerbar
Copy link
Member Author

Choose a reason for hiding this comment

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

This shouldn't be in headerbar I don't think....

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. I forgot to remove this after implementing i18n. It should be moved to the Storybook part since it simulates the App role.

Copy link
Member Author

@amcgee amcgee May 27, 2019

Choose a reason for hiding this comment

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

Should we do that in another PR then?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, just leave it as it is with the TODO and we can do it in a different PR.

src/HeaderBar/index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@varl varl 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. Is it a draft because the related PR needs a merge first or is there other work you want to do on it?

@varl
Copy link
Contributor

varl commented May 27, 2019

I get an error on the Netlify deploy preview: https://deploy-preview-7--dhis2-ui-widgets.netlify.com/?path=/story/headerbar--loading: ERROR: customData is undefined

@amcgee
Copy link
Member Author

amcgee commented May 27, 2019

Yep, the Storybook error on Netlify and the drafty nature of this PR are both because of the prerequisite app-runtime PR

Copy link
Member

@cooper-joe cooper-joe left a comment

Choose a reason for hiding this comment

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

Nice! Looks good. I'll have a think about a possible loading state.

@amcgee
Copy link
Member Author

amcgee commented May 27, 2019

@varl upgraded app-runtime within this PR too since greenkeeper isn't enabled (#2), now Storybook should be good to go on Netlify

@amcgee amcgee marked this pull request as ready for review May 27, 2019 12:30
@varl
Copy link
Contributor

varl commented May 27, 2019

Works!

And this repo is semantic release enabled so will roll out when you merge it.

@amcgee amcgee merged commit eae0a16 into master May 28, 2019
@amcgee
Copy link
Member Author

amcgee commented May 28, 2019

Merged! @varl might recommend forcing squash-and-merge, doesn't appear to be required in this repo.

@amcgee amcgee deleted the fix/better-loading-appearance branch May 28, 2019 08:46
dhis2-bot pushed a commit that referenced this pull request May 28, 2019
## [1.0.3](v1.0.2...v1.0.3) (2019-05-28)

### Bug Fixes

* don't break the page structure while loading data for the headerbar ([#7](#7)) ([eae0a16](eae0a16))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 1.0.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants