mozilla / perfcompare

Improved Performance Comparison Tool
Mozilla Public License 2.0
39 stars 93 forks source link

moved framework dropdown to results table #691

Closed Carla-Moz closed 2 months ago

Carla-Moz commented 2 months ago

This PR closes the following issue: https://mozilla-hub.atlassian.net/browse/PCF-421

This PR is broken down into 3 commits:

Screenshot 2024-07-23 at 12 20 37

To Test: 1.) Open the PR's deployed link: https://deploy-preview-691--mozilla-perfcompare.netlify.app/ 2.) Select a base revision and new revision(s) in the Compare with a base component 3.) After Results page has finished loading, change the framework in the menu bar above the Results table.

  1. ) Spinner should appear as the table loads and disappears when finished loading new results.
  2. ) Click Edit entry and change one of the revisions.
  3. ) Press Compare
  4. ) The framework value should not change and is not missing in the url.
  5. ) Repeat with Compare over time
netlify[bot] commented 2 months ago

Deploy Preview for mozilla-perfcompare ready!

Name Link
Latest commit 35d2e58e32f2b595ef7c9a04ddab488906d2b3ed
Latest deploy log https://app.netlify.com/sites/mozilla-perfcompare/deploys/66b243a195cb7b00081a62ac
Deploy Preview https://deploy-preview-691--mozilla-perfcompare.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 88.59%. Comparing base (d6451de) to head (a524b6f).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## beta #691 +/- ## ========================================== + Coverage 88.40% 88.59% +0.18% ========================================== Files 75 76 +1 Lines 1889 1920 +31 Branches 348 358 +10 ========================================== + Hits 1670 1701 +31 Misses 185 185 Partials 34 34 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

julienw commented 2 months ago

But with these changes I see that "Download JSON" button gets a bit shorter and therefore it becomes two lines.

I guess the "Download JSON" button (or rather its container) could be flex: none.

Carla-Moz commented 2 months ago

@julienw,

resolved comments commit

memoize the revisions commit

Same page updates are quicker, but no spinner. I guess we can leave it as is since the Compare/Cancelbuttons disappear when the results are resolved. However, there's no feedback when updating the framework. shrug Let me know what you think. I could have done something wrong.

Carla-Moz commented 2 months ago

@julienw, resolved your comments here. I think this is in a good shape and we can merge soon. Will make a separate PR for the styles of the row to fit all the dropdown better and for the tests.

julienw commented 2 months ago

@Carla-Moz I'm sorry about that, but I think you merged the beta branch incorrectly to your branch :/ now a lot of things are showing up in this PR... I think you can recover by using git rebase -i upstream/beta and manually removing the lines that don't belong to your patch (the ones coming from Beatrice and myself).

Also please please please remove all changes related to Suspense and the defer in the loaders (this means: please add defer back, as I'm removing it in the other PR), and only focus on the framework stuff. A lot of the complexity in this patch comes from the fact you tried to solve 2 problems in one. This is also failing the tests.

If you can solve it while I'm away this afternoon I promise I'll look at it again when I come back later today.

Carla-Moz commented 2 months ago

That unfortunate. Closing this PR. Please see https://github.com/mozilla/perfcompare/pull/704 - has all the changes requested, green tests, and clean rebase