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

Feature/lazy loading #1041

Merged
merged 34 commits into from
Sep 7, 2022
Merged

Feature/lazy loading #1041

merged 34 commits into from
Sep 7, 2022

Conversation

Huongg
Copy link
Contributor

@Huongg Huongg commented Aug 31, 2022

Description

Fixes #1005

Development notes

I've created 2 loaders components for both meta-data and tracking-data.

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

Huongg added 4 commits August 26, 2022 11:17
Signed-off-by: huongg <huongg1409@gmail.com>
Signed-off-by: huongg <huongg1409@gmail.com>
Signed-off-by: huongg <huongg1409@gmail.com>
…zy-loading

Signed-off-by: huongg <huongg1409@gmail.com>
Huongg added 4 commits August 31, 2022 10:59
Signed-off-by: huongg <huongg1409@gmail.com>
Signed-off-by: huongg <huongg1409@gmail.com>
Signed-off-by: huongg <huongg1409@gmail.com>
Signed-off-by: huongg <huongg1409@gmail.com>
@comym
Copy link

comym commented Sep 1, 2022

hi @Huongg just had a look at it now

It was actually hard to keep the lazy loading running for long to observe it in detail even when switching to a slow 3g connection.

It looks alright to me and my two comments are:

We should try to make a better alignment/positioning of the rectangles to align as perfect as possible to the text data positioning when it appears.

The screenshot below shows the two overlayed. You can see they are not aligned and what happens is when we suddenly move from rectangles to text, there's a jump in positioning. Basically, the expectation of something appearing at a certain position is not matched and there's the impression that the content is moving. I'm aware the rectangles are just a representation of possible content with lines and spacing in between and might never match it perfectly depending on the data we're loading. However, especially the vertical alignment of the columns is important for us to nail.

Have a look:

Screenshot 2022-09-01 at 13 08 44
Screenshot 2022-09-01 at 13 08 35
Screenshot 2022-09-01 at 13 08 27

Lastly, the colours of the pulsating rectangle are alright, however the fact that it's visible every time you click on a run for a split second makes everything in my opinion annoyingly jumpy. Cause it ends up not being something that only occasionally happens, but it happens every time. The only time that it does not happen is when you go back to a run that you actually loaded previously because it's already loaded.

I thought making the lazy loading rectangles have a more subtle colour would help at minimizing this effect and make everything look less jumpy, but I don't think it worked. We can keep the colours as it is for now. I do think it's better though. The contrast between background and foreground is less now and better for the eyes.

I don't know how to proceed from here. If going back to the idea that @tynandebold mentioned before, of creating some sort of delay - in a less hacky way - so users don't see the lazy loading appearing every single time you click on something, but only when is really necessary.

Technically, I leave it to you guys, but that would be the way to go in my opinion. Let's keep the conversation going and I'm happy to hear from the team what the best solution could be. Thanks!

@Huongg Huongg self-assigned this Sep 1, 2022
Signed-off-by: huongg <huongg1409@gmail.com>
Signed-off-by: huongg <huongg1409@gmail.com>
Signed-off-by: huongg <huongg1409@gmail.com>
…zy-loading

Signed-off-by: huongg <huongg1409@gmail.com>
Signed-off-by: huongg <huongg1409@gmail.com>
Signed-off-by: huongg <huongg1409@gmail.com>
Copy link
Contributor

@rashidakanchwala rashidakanchwala left a comment

Choose a reason for hiding this comment

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

Looks great, i can see the alignment is correct in comparison mode. thanks @Huongg

Signed-off-by: huongg <huongg1409@gmail.com>
Signed-off-by: huongg <huongg1409@gmail.com>
Copy link
Member

@tynandebold tynandebold left a comment

Choose a reason for hiding this comment

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

This is looking great and really brings the feel of the app up to a new level!

I'm going to request changes instead of approving just because of the number of comments I have. None of them are major though, so they should be able to be remedied without much effort.

A few other points I noticed while testing:

  1. At one point I saw the tracking data loader rows were very short:
    Screen Shot 2022-09-05 at 14 01 31
  2. In comparison mode, the metadata and tracking data aren't lining up. Maybe this is still a WIP?
    Screen Shot 2022-09-05 at 17 42 19
  3. The colors of the light theme have very subtle and for me a hard-to-see contrast. Should we tweak that?
  4. I would speed up the animation. Maybe just use the default value, which is think is 1.2, unless design has specifically asked for this :)

Signed-off-by: huongg <huongg1409@gmail.com>
Signed-off-by: huongg <huongg1409@gmail.com>
Signed-off-by: huongg <huongg1409@gmail.com>
Signed-off-by: huongg <huongg1409@gmail.com>
Signed-off-by: huongg <huongg1409@gmail.com>
Signed-off-by: huongg <huongg1409@gmail.com>
@Huongg
Copy link
Contributor Author

Huongg commented Sep 6, 2022

3. The colors of the light theme have very subtle and for me a hard-to-see contrast

Hiya, thanks for your comments. For points 1 and 2, I didn't push my the latest last night so what you saw was prob the older version, sorry. I've asked Gabriel about the colour you mentioned in point 3, happy to tweak it. And I also remove the speed to set it back to the default value.

For other comments, I've addressed them and left some comments with my changes 😄

…zy-loading

Signed-off-by: huongg <huongg1409@gmail.com>
Signed-off-by: huongg <huongg1409@gmail.com>
Copy link
Member

@tynandebold tynandebold left a comment

Choose a reason for hiding this comment

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

Fantastic work! Thank you for making the changes.

src/components/experiment-tracking/details/details.js Outdated Show resolved Hide resolved
src/components/experiment-wrapper/experiment-wrapper.js Outdated Show resolved Hide resolved
Signed-off-by: huongg <huongg1409@gmail.com>
Signed-off-by: huongg <huongg1409@gmail.com>
@Huongg
Copy link
Contributor Author

Huongg commented Sep 6, 2022

thank you @tynandebold.

@comym just reminded me about removing the sliding effect in the comparison view...I completely forgot we agreed we would do that. I've removed the sliding in effect to make it work nicer with the lazy loading now.

@Huongg Huongg requested a review from comym September 6, 2022 12:50
@tynandebold
Copy link
Member

thank you @tynandebold.

@comym just reminded me about removing the sliding effect in the comparison view...I completely forgot we agreed we would do that. I've removed the sliding in effect to make it work nicer with the lazy loading now.

I actually think we should keep the sliding effect. Given that it's a safe bet the user is very rarely going to see the loading components, don't we still want the new data to slide in when it appears?

In those rare instances when something is taking ages to load, yes the user will see the loading indicators and then the data will slide in, but that will be a very small percentage of total views. Again, the majority of the time we won't see the loading indicators, and if we don't see them and then we remove the slide in, we lose that lovely animation and moment of delight.

Signed-off-by: huongg <huongg1409@gmail.com>
@Huongg Huongg requested a review from yetudada as a code owner September 6, 2022 13:27
RELEASE.md Outdated Show resolved Hide resolved
Huongg and others added 4 commits September 6, 2022 14:32
Co-authored-by: Tynan DeBold <thdebold@gmail.com>
Signed-off-by: huongg <huongg1409@gmail.com>
Signed-off-by: huongg <huongg1409@gmail.com>
Signed-off-by: huongg <huongg1409@gmail.com>
Copy link
Contributor

@cvetankanechevska cvetankanechevska left a comment

Choose a reason for hiding this comment

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

Looks great to me and I agree with Tynan of having the slide in animation.

@comym
Copy link

comym commented Sep 6, 2022

Hi, I've just checked it with the increased delay time and the sliding effect added back

Let's keep the sliding effect, as it is. Let's keep it.

However I still see the header and content columns moving up and down as described here
#1005 (comment)

Pretty much there
That's all for now, many thanks

…layout changing

Signed-off-by: huongg <huongg1409@gmail.com>
Signed-off-by: huongg <huongg1409@gmail.com>
@Huongg
Copy link
Contributor Author

Huongg commented Sep 6, 2022

awesome thank you @comym @cvetankanechevska @rashidakanchwala and @tynandebold. Lets keep the sliding in effect then. I've updated the delay timing to 200ms so the gitpod looks in sync with our local version, but also not too slow for data to load

For your feedback about the layout shifting @comym, thanks for noticing it. My latest changes should address them now. You can now review again on the gitpod 😄

foregroundLightTheme: '#EAEAEA',
backgroundDarkTheme: '#071d28',
foregroundDarkTheme: '#20313a',
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Just one small adjustment, is this formatted with prettier? If so I think all color values will be lower cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yeah thanks for spotting it, im meant to update with your colour variables but completely forgot again.

@cvetankanechevska
Copy link
Contributor

awesome thank you @comym @cvetankanechevska @rashidakanchwala and @tynandebold. Lets keep the sliding in effect then. I've updated the delay timing to 200ms so the gitpod looks in sync with our local version, but also not too slow for data to load

For your feedback about the layout shifting @comym, thanks for noticing it. My latest changes should address them now. You can now review again on the gitpod 😄

Great work Huong! 🎉

Copy link

@comym comym left a comment

Choose a reason for hiding this comment

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

Hi @Huongg looks and feels great. I'm approving it.

As the last thing, I just realised that we were not using our current colour palette for the lazy loading gradient colours.

Please make sure to follow:

Dark theme:
Start: Slate-600
End: Slate-200

Light theme:
Start: Grey-200
End: Grey-100

Many thanks :)

Signed-off-by: huongg <huongg1409@gmail.com>
@Huongg Huongg merged commit 06fb743 into main Sep 7, 2022
@Huongg Huongg deleted the feature/lazy-loading branch September 7, 2022 08:29
@tynandebold tynandebold mentioned this pull request Sep 8, 2022
5 tasks
jmholzer pushed a commit that referenced this pull request Sep 8, 2022
* loader components

Signed-off-by: huongg <huongg1409@gmail.com>

* test: passing props around for the loader components

Signed-off-by: huongg <huongg1409@gmail.com>

* apply dark and light themes colours

Signed-off-by: huongg <huongg1409@gmail.com>

* remove commented code

Signed-off-by: huongg <huongg1409@gmail.com>

* apply new gradients colours

Signed-off-by: huongg <huongg1409@gmail.com>

* use variables for colours

Signed-off-by: huongg <huongg1409@gmail.com>

* clear console error

Signed-off-by: huongg <huongg1409@gmail.com>

* add delay timing to show loaders

Signed-off-by: huongg <huongg1409@gmail.com>

* adjust alignments for meta-data

Signed-off-by: huongg <huongg1409@gmail.com>

* adjust alignments for tracking-data

Signed-off-by: huongg <huongg1409@gmail.com>

* alignments for third run

Signed-off-by: huongg <huongg1409@gmail.com>

* adjust the gap between loaders

Signed-off-by: huongg <huongg1409@gmail.com>

* store variables in config file instead

Signed-off-by: huongg <huongg1409@gmail.com>

* fix the gap for tracking data

Signed-off-by: huongg <huongg1409@gmail.com>

* update states name and comments

Signed-off-by: huongg <huongg1409@gmail.com>

* move the small loaders component to be in loaders file

Signed-off-by: huongg <huongg1409@gmail.com>

* removed unused props

Signed-off-by: huongg <huongg1409@gmail.com>

* move gap value to the config file

Signed-off-by: huongg <huongg1409@gmail.com>

* set speed back to the default value

Signed-off-by: huongg <huongg1409@gmail.com>

* update value for tracking data

Signed-off-by: huongg <huongg1409@gmail.com>

* update light theme colours

Signed-off-by: huongg <huongg1409@gmail.com>

* remove the fading in effect in comparison mode

Signed-off-by: huongg <huongg1409@gmail.com>

* nit sorting

Signed-off-by: huongg <huongg1409@gmail.com>

* update release note

Signed-off-by: huongg <huongg1409@gmail.com>

* Update RELEASE.md

Co-authored-by: Tynan DeBold <thdebold@gmail.com>

* remove colour refactoring from the release note

Signed-off-by: huongg <huongg1409@gmail.com>

* increase the time to 500 for testing on gitpod

Signed-off-by: huongg <huongg1409@gmail.com>

* add the sliding animation back in

Signed-off-by: huongg <huongg1409@gmail.com>

* add max-height for metadata and viewbox for tracking data to prevent layout changing

Signed-off-by: huongg <huongg1409@gmail.com>

* update delay timing to 200

Signed-off-by: huongg <huongg1409@gmail.com>

* use colour variables from variables file

Signed-off-by: huongg <huongg1409@gmail.com>

Signed-off-by: huongg <huongg1409@gmail.com>
Co-authored-by: Tynan DeBold <thdebold@gmail.com>
Signed-off-by: Jannic <jannic.holzer@gmail.com>
jmholzer pushed a commit that referenced this pull request Sep 16, 2022
* loader components

Signed-off-by: huongg <huongg1409@gmail.com>

* test: passing props around for the loader components

Signed-off-by: huongg <huongg1409@gmail.com>

* apply dark and light themes colours

Signed-off-by: huongg <huongg1409@gmail.com>

* remove commented code

Signed-off-by: huongg <huongg1409@gmail.com>

* apply new gradients colours

Signed-off-by: huongg <huongg1409@gmail.com>

* use variables for colours

Signed-off-by: huongg <huongg1409@gmail.com>

* clear console error

Signed-off-by: huongg <huongg1409@gmail.com>

* add delay timing to show loaders

Signed-off-by: huongg <huongg1409@gmail.com>

* adjust alignments for meta-data

Signed-off-by: huongg <huongg1409@gmail.com>

* adjust alignments for tracking-data

Signed-off-by: huongg <huongg1409@gmail.com>

* alignments for third run

Signed-off-by: huongg <huongg1409@gmail.com>

* adjust the gap between loaders

Signed-off-by: huongg <huongg1409@gmail.com>

* store variables in config file instead

Signed-off-by: huongg <huongg1409@gmail.com>

* fix the gap for tracking data

Signed-off-by: huongg <huongg1409@gmail.com>

* update states name and comments

Signed-off-by: huongg <huongg1409@gmail.com>

* move the small loaders component to be in loaders file

Signed-off-by: huongg <huongg1409@gmail.com>

* removed unused props

Signed-off-by: huongg <huongg1409@gmail.com>

* move gap value to the config file

Signed-off-by: huongg <huongg1409@gmail.com>

* set speed back to the default value

Signed-off-by: huongg <huongg1409@gmail.com>

* update value for tracking data

Signed-off-by: huongg <huongg1409@gmail.com>

* update light theme colours

Signed-off-by: huongg <huongg1409@gmail.com>

* remove the fading in effect in comparison mode

Signed-off-by: huongg <huongg1409@gmail.com>

* nit sorting

Signed-off-by: huongg <huongg1409@gmail.com>

* update release note

Signed-off-by: huongg <huongg1409@gmail.com>

* Update RELEASE.md

Co-authored-by: Tynan DeBold <thdebold@gmail.com>

* remove colour refactoring from the release note

Signed-off-by: huongg <huongg1409@gmail.com>

* increase the time to 500 for testing on gitpod

Signed-off-by: huongg <huongg1409@gmail.com>

* add the sliding animation back in

Signed-off-by: huongg <huongg1409@gmail.com>

* add max-height for metadata and viewbox for tracking data to prevent layout changing

Signed-off-by: huongg <huongg1409@gmail.com>

* update delay timing to 200

Signed-off-by: huongg <huongg1409@gmail.com>

* use colour variables from variables file

Signed-off-by: huongg <huongg1409@gmail.com>

Signed-off-by: huongg <huongg1409@gmail.com>
Co-authored-by: Tynan DeBold <thdebold@gmail.com>
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.

Explore lazy loading results in the experiment tracking view
5 participants