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(chartGraph): convert graph data, add error/loading states #40

Merged
merged 1 commit into from
Jul 19, 2019

Conversation

priley86
Copy link
Contributor

@priley86 priley86 commented Jul 18, 2019

What's included

  • parse the Redux date and sockets inputs and generate chart data and
    tooltips
  • add error state and zeroed array
  • loading state

Outstanding question - should a max-y value be set for non error state?

How to test

Example

Loading desktop:
loading-desktop

Loading mobile:
loading-mobile

Loading complete:
loading-full

Error state (zeroed array), max-y set to 100, toast notification to come in future PR... :
Screen Shot 2019-07-18 at 11 43 03 AM

Updates issue/story

Closes #18

Copy link
Member

@cdcabrera cdcabrera left a comment

Choose a reason for hiding this comment

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

Made a commit with the internal review changes. Give it once over. We probably need to

  • look at moving the x and y axis defaults into the helpers due to needing additional logic in the event a valid array result set is returned with zero results...
  • also we may need to look at filling out a full 30 days in the event a partial array result set is returned.. but we can actually test that against the API through the proxy

src/components/rhelGraphCard/rhelGraphCard.js Show resolved Hide resolved
@@ -246,125 +246,125 @@ exports[`RhelGraphCard Component should render a non-connected component: non-co
exports[`RhelGraphCard Component should render multiple states: error shows zeroed bar values 1`] = `
Object {
"chartBarData": Array [
Object {
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'm good after this bit is updated... i believe it should start w/ Jun 1 if Jun 1 UTC is passed.

Copy link
Member

@cdcabrera cdcabrera Jul 18, 2019

Choose a reason for hiding this comment

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

ugh missed that...

edit:
it's part of the conversion that's happening as the default props get replaced. sooooo it looks like that needs to be offset inside the helpers if we want to continue passing a date type... otherwise we'll end up being inconsistent and sometimes passing an instance of moment or in the case of what needs to happen for the component an instance of date ... I'll refactor the helpers so the moment aspect is contained and the component is merely a passthrough

edit:
so technically the output of this is accurate, that's what is produced based off of a new Date('2019-06-01T00:00:00Z'), and when run in our environments a local output ... I was silly, and missed a utc conversion in the graphHelpers, hence the offset was being included

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh... yes, you are correct:

new Date('2019-06-01T00:00:00Z')
Fri May 31 2019 20:00:00 GMT-0400 (Eastern Daylight Time)

Ok, yes, if sourced from the server....shouldn't matter. I am guessing that Jest uses the system UTC time (so EST in my case), but not positive.

Maybe some way to mock this better in the future? FWIW, it's not obvious. Could also just set to something same day in UTC /U.S. time zones for being more obvious...

new Date('2019-06-01T12:00:00Z')
Sat Jun 01 2019 08:00:00 GMT-0400 (Eastern Daylight Time)

@cdcabrera cdcabrera force-pushed the issues/18 branch 2 times, most recently from 8514db0 to ba10156 Compare July 19, 2019 04:19
Copy link
Member

@cdcabrera cdcabrera left a comment

Choose a reason for hiding this comment

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

Tests are passing, UTC implementation appears to be behaving on date render. I'm good if you're good.

Double check your commit message when you're ready, squash em, and make sure to update your commit subject with the issues/18 or the PR number, or both. Looks like when the squash was disabled that was nixed too, at least on my last commit...

  • feat(chartGraph): convert graph data, add error/loading states (#40)

…tInsights#40)

* parse the Redux date and sockets inputs and generate chart data and
tooltips
* add error state and zeroed array
* loading skeletor
* graphHelpers, rhelGraphCard replace translate refs
* rhelGraphCard, consolidate logic
* update integration snapshots
* graphHelpers, minor UTC correction, snapshot corrections
* dateHelpers, initial date defaults, prep for additional date functions
* helpers, index layout
* rhelGraphCard, replace default startDate, endDate props with helper
@codecov-io
Copy link

codecov-io commented Jul 19, 2019

Codecov Report

Merging #40 into master will increase coverage by 1.26%.
The diff coverage is 97.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #40      +/-   ##
==========================================
+ Coverage   84.55%   85.81%   +1.26%     
==========================================
  Files          22       23       +1     
  Lines         246      275      +29     
  Branches       53       59       +6     
==========================================
+ Hits          208      236      +28     
- Misses         30       31       +1     
  Partials        8        8
Impacted Files Coverage Δ
src/services/rhelServices.js 100% <ø> (ø) ⬆️
src/components/rhelGraphCard/rhelGraphCard.js 89.28% <100%> (+1.28%) ⬆️
src/common/dateHelpers.js 100% <100%> (ø)
src/common/graphHelpers.js 97.87% <96.29%> (-2.13%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66f34ef...d4ad093. Read the comment docs.

@cdcabrera cdcabrera merged commit da2158a into RedHatInsights:master Jul 19, 2019
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.

Dashboard graph card helpers
3 participants