Skip to content

Add loading for initial state on graph#5448

Merged
bramkragten merged 14 commits intohome-assistant:devfrom
timmo001:initial-loading
Apr 15, 2020
Merged

Add loading for initial state on graph#5448
bramkragten merged 14 commits intohome-assistant:devfrom
timmo001:initial-loading

Conversation

@timmo001
Copy link
Copy Markdown
Member

@timmo001 timmo001 commented Apr 4, 2020

Proposed change

When loading history for the graph initially, there is a window of time where the graph states there is no state history, whilst the data is being loaded initially. This adds a spinner to be shown whilst this is loading.

loading
This is only usually seen by the human eye when you have a lot of cards on a production server. This gif is using a setTimeout to slow this down as the local dev instance is much quicker.

Type of change

  • New feature (thank you!)

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

@timmo001 timmo001 changed the title Add loading for initial state on sensor graph Add loading for initial state on graph Apr 4, 2020
@property() protected _config?: GraphHeaderFooterConfig;
@property() private _coordinates?: any;
@property() private _coordinates?: number[][];
@property() private _loadingInitialData: boolean = true;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this need to be a property? When coordinates changes it should update right?

It may not request an update if its set to undefined though. Not 100%. But you may not need this to be a property.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point. Made a non-property

@bramkragten
Copy link
Copy Markdown
Member

Can we give the spinner div the same height as the graph to prevent jumping?

@bramkragten
Copy link
Copy Markdown
Member

We should also not load new history data while another request is still running, we now start a new request every minute even if the old one is not finished. But that is for another PR.

@bramkragten
Copy link
Copy Markdown
Member

It works weird anyway, it will update when a property coincidentally (hass?) changed and the data was fetched longer than 1 minute ago?

@timmo001
Copy link
Copy Markdown
Member Author

timmo001 commented Apr 8, 2020

OK done

height: 58px;
text-align: center;
display: flex;
height: 98px;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The height is not fixed, it is 1/5 of the width.

Copy link
Copy Markdown
Member

@bramkragten bramkragten Apr 8, 2020

Choose a reason for hiding this comment

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

You can work with margin-bottom: 20% and give the paper-spinner position: absolute;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah OK. I'll fix now

@timmo001
Copy link
Copy Markdown
Member Author

timmo001 commented Apr 8, 2020

OK that worked.

Screenshot_20200408_171551

Screenshot_20200408_171922

@zsarnett
Copy link
Copy Markdown
Contributor

zsarnett commented Apr 8, 2020

Looking good! Can we center the spinner vertically?

@timmo001
Copy link
Copy Markdown
Member Author

timmo001 commented Apr 8, 2020

Looking good! Can we center the spinner vertically?

Tried it, but the spinner seems to want to center itself in the card itself instead of its container

@zsarnett
Copy link
Copy Markdown
Contributor

zsarnett commented Apr 8, 2020

@timmo001 Make the Spinner Container position relative and try again

@timmo001
Copy link
Copy Markdown
Member Author

timmo001 commented Apr 8, 2020

No change 😕

Screenshot_20200408_180009

      .spinner-container {
        position: relative;
        margin-bottom: 20%;
      }
      .spinner {
        position: absolute;
        top: calc(50% - 12px);
        left: calc(50% - 12px);
      }

Gotta love css

@zsarnett
Copy link
Copy Markdown
Contributor

zsarnett commented Apr 8, 2020

Instead of margin-bottom: 20% use padding.

@timmo001
Copy link
Copy Markdown
Member Author

timmo001 commented Apr 8, 2020

Instead of margin-bottom: 20% use padding.

That did it. Cheers 🍻

Screenshot_20200408_181321

That extra space above is from the entity section which uses 16px padding top and bottom

@timmo001
Copy link
Copy Markdown
Member Author

timmo001 commented Apr 8, 2020

Actually....

Screenshot_20200408_181627

There done 😄

@property() private _coordinates?: any;
@property() private _coordinates?: number[][];

private _loadingInitialData: boolean = true;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of doing this we should update the coordinates function. Instead of returning undefined when there is no state history we should return an empty array. That way we don't need an extra variable.

If _coordinates is undefined we know we haven't updated from the state history yet. But if the Length is of coordinates is 0 then no state history found

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@bramkragten What do you think of this?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Or use undefined and null

@timmo001
Copy link
Copy Markdown
Member Author

timmo001 commented Apr 9, 2020

OK that's done. I also made the no history message match the height/position of the rest

Comment thread src/panels/lovelace/header-footer/hui-graph-header-footer.ts Outdated
Comment thread src/panels/lovelace/header-footer/hui-graph-header-footer.ts Outdated
Comment thread src/panels/lovelace/header-footer/hui-graph-header-footer.ts Outdated
@timmo001 timmo001 requested a review from bramkragten April 10, 2020 15:05
Comment thread src/panels/lovelace/header-footer/hui-graph-header-footer.ts Outdated
Comment thread src/panels/lovelace/header-footer/hui-graph-header-footer.ts
return css`
paper-spinner {
position: absolute;
top: calc(50% - 28px);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You do realize this isn't the center of the container right? The center would be calc(50% - 14px).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This takes into account half of the spinner size, otherwise, you get this:
Screenshot_20200415_170910

timmo001 and others added 3 commits April 15, 2020 17:06
Co-Authored-By: Bram Kragten <mail@bramkragten.nl>
Co-Authored-By: Bram Kragten <mail@bramkragten.nl>
@timmo001 timmo001 requested a review from bramkragten April 15, 2020 16:12
@bramkragten bramkragten merged commit ff81536 into home-assistant:dev Apr 15, 2020
@timmo001 timmo001 deleted the initial-loading branch April 15, 2020 18:54
@lock lock Bot locked and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants