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

@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
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

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 :)

@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 😄

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
@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 <[email protected]>
@Huongg Huongg requested a review from yetudada as a code owner September 6, 2022 13:27
RELEASE.md Outdated Show resolved Hide resolved
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

@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 :)

@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 <[email protected]>

* test: passing props around for the loader components

Signed-off-by: huongg <[email protected]>

* apply dark and light themes colours

Signed-off-by: huongg <[email protected]>

* remove commented code

Signed-off-by: huongg <[email protected]>

* apply new gradients colours

Signed-off-by: huongg <[email protected]>

* use variables for colours

Signed-off-by: huongg <[email protected]>

* clear console error

Signed-off-by: huongg <[email protected]>

* add delay timing to show loaders

Signed-off-by: huongg <[email protected]>

* adjust alignments for meta-data

Signed-off-by: huongg <[email protected]>

* adjust alignments for tracking-data

Signed-off-by: huongg <[email protected]>

* alignments for third run

Signed-off-by: huongg <[email protected]>

* adjust the gap between loaders

Signed-off-by: huongg <[email protected]>

* store variables in config file instead

Signed-off-by: huongg <[email protected]>

* fix the gap for tracking data

Signed-off-by: huongg <[email protected]>

* update states name and comments

Signed-off-by: huongg <[email protected]>

* move the small loaders component to be in loaders file

Signed-off-by: huongg <[email protected]>

* removed unused props

Signed-off-by: huongg <[email protected]>

* move gap value to the config file

Signed-off-by: huongg <[email protected]>

* set speed back to the default value

Signed-off-by: huongg <[email protected]>

* update value for tracking data

Signed-off-by: huongg <[email protected]>

* update light theme colours

Signed-off-by: huongg <[email protected]>

* remove the fading in effect in comparison mode

Signed-off-by: huongg <[email protected]>

* nit sorting

Signed-off-by: huongg <[email protected]>

* update release note

Signed-off-by: huongg <[email protected]>

* Update RELEASE.md

Co-authored-by: Tynan DeBold <[email protected]>

* remove colour refactoring from the release note

Signed-off-by: huongg <[email protected]>

* increase the time to 500 for testing on gitpod

Signed-off-by: huongg <[email protected]>

* add the sliding animation back in

Signed-off-by: huongg <[email protected]>

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

Signed-off-by: huongg <[email protected]>

* update delay timing to 200

Signed-off-by: huongg <[email protected]>

* use colour variables from variables file

Signed-off-by: huongg <[email protected]>

Signed-off-by: huongg <[email protected]>
Co-authored-by: Tynan DeBold <[email protected]>
Signed-off-by: Jannic <[email protected]>
jmholzer pushed a commit that referenced this pull request Sep 16, 2022
* loader components

Signed-off-by: huongg <[email protected]>

* test: passing props around for the loader components

Signed-off-by: huongg <[email protected]>

* apply dark and light themes colours

Signed-off-by: huongg <[email protected]>

* remove commented code

Signed-off-by: huongg <[email protected]>

* apply new gradients colours

Signed-off-by: huongg <[email protected]>

* use variables for colours

Signed-off-by: huongg <[email protected]>

* clear console error

Signed-off-by: huongg <[email protected]>

* add delay timing to show loaders

Signed-off-by: huongg <[email protected]>

* adjust alignments for meta-data

Signed-off-by: huongg <[email protected]>

* adjust alignments for tracking-data

Signed-off-by: huongg <[email protected]>

* alignments for third run

Signed-off-by: huongg <[email protected]>

* adjust the gap between loaders

Signed-off-by: huongg <[email protected]>

* store variables in config file instead

Signed-off-by: huongg <[email protected]>

* fix the gap for tracking data

Signed-off-by: huongg <[email protected]>

* update states name and comments

Signed-off-by: huongg <[email protected]>

* move the small loaders component to be in loaders file

Signed-off-by: huongg <[email protected]>

* removed unused props

Signed-off-by: huongg <[email protected]>

* move gap value to the config file

Signed-off-by: huongg <[email protected]>

* set speed back to the default value

Signed-off-by: huongg <[email protected]>

* update value for tracking data

Signed-off-by: huongg <[email protected]>

* update light theme colours

Signed-off-by: huongg <[email protected]>

* remove the fading in effect in comparison mode

Signed-off-by: huongg <[email protected]>

* nit sorting

Signed-off-by: huongg <[email protected]>

* update release note

Signed-off-by: huongg <[email protected]>

* Update RELEASE.md

Co-authored-by: Tynan DeBold <[email protected]>

* remove colour refactoring from the release note

Signed-off-by: huongg <[email protected]>

* increase the time to 500 for testing on gitpod

Signed-off-by: huongg <[email protected]>

* add the sliding animation back in

Signed-off-by: huongg <[email protected]>

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

Signed-off-by: huongg <[email protected]>

* update delay timing to 200

Signed-off-by: huongg <[email protected]>

* use colour variables from variables file

Signed-off-by: huongg <[email protected]>

Signed-off-by: huongg <[email protected]>
Co-authored-by: Tynan DeBold <[email protected]>
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