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

Map loader + Chart loader + sourceLink on chart container #44

Merged
merged 7 commits into from Aug 22, 2019

Conversation

karimkawambwa
Copy link
Contributor

@karimkawambwa karimkawambwa commented Aug 21, 2019

Description

Video: https://recordit.co/8XI1wzJpDo
The gif below lost quality, not good representation.

Screen Shot 2019-08-21 at 4 36 13 PM

Screen Shot 2019-08-21 at 4 36 37 PM

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation

@karimkawambwa karimkawambwa self-assigned this Aug 21, 2019
@karimkawambwa
Copy link
Contributor Author

@kilemensi looks good ?

karimkawambwa added a commit to CodeForAfrica/Dominion.AFRICA that referenced this pull request Aug 21, 2019
Copy link
Member

@kilemensi kilemensi left a comment

Choose a reason for hiding this comment

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

Looks and works great @karimkawambwa


  1. Code-wise, there is too much
{loading ? (
            <ContentLoader
              primaryOpacity={0.5}
              secondaryOpacity={1}
          >
            >
              <rect x="0" y="0" width="100%" height="100%" />
            </ContentLoader>

Customize it like you've done for TypographyLoader. e.g.

<BlockLoader loading={loading} {...props}>
   {content to show when content is loaded}
</BlockLoader>
  1. Lets move sources to individual charts.

src/ChartContainer/index.js Show resolved Hide resolved
@karimkawambwa karimkawambwa changed the title Chart container loader Map loader + Chart loader + sourceLink on chart container Aug 21, 2019
@karimkawambwa
Copy link
Contributor Author

  1. Lets move sources to individual charts.

There will be only one source link @kilemensi

Screen Shot 2019-08-21 at 5 38 29 PM

cc @DavidLemayian

@karimkawambwa
Copy link
Contributor Author

@kilemensi I have addressed the code

Copy link
Member

@kilemensi kilemensi left a comment

Choose a reason for hiding this comment

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

src/BlockLoader.js Outdated Show resolved Hide resolved
src/BlockLoader.js Outdated Show resolved Hide resolved
<BlockLoader loading width="100%" height="100%" />
</div>
)}
<div
Copy link
Member

Choose a reason for hiding this comment

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

This couldn't be a <BlockLoader ...><div ... /></BlockLoader>

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 didn't do this because of the root div.
Also because initial loading will look for div with mapId.
I will track this am make enhancement to the code later.

@karimkawambwa karimkawambwa merged commit 6767701 into master Aug 22, 2019
@karimkawambwa karimkawambwa deleted the loaders branch August 22, 2019 05:34
karimkawambwa added a commit to CodeForAfrica/Dominion.AFRICA that referenced this pull request Aug 27, 2019
* Initial skeleton loader

* Fix loader look

* Profile tab loader

* Search bar skeleton loader + all static text

* Set is loading for chart container skeleton from hurumap-ui CodeForAfrica/HURUmap-UI#44

* Bump hurumap-ui version

* Fix title grid

* Fix chart factory to new hurumap-ui requirements
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.

None yet

2 participants