mozilla / perfcompare

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

enhancement: Make results tiltle editable and URL persistent #774

Open FatumaA opened 5 days ago

FatumaA commented 5 days ago

This PR makes the Results page title editable and persists in the URL state.

Before, the Results view had "Results" as the title. This title could not be customized and was not part of the URL parameters.

Now:

A screenshot of the results page with an arrow pointing to the title and a thick red line below the URL 
params at the top of the image

Before the changes:

A screenshot of the results page with an arrow pointing to the title and a thick red line below the URL 
params at the top of the image

Closes #769

netlify[bot] commented 5 days ago

Deploy Preview for mozilla-perfcompare ready!

Name Link
Latest commit ffddac0a8c054e2c0fccb41217da5435b517a4e2
Latest deploy log https://app.netlify.com/sites/mozilla-perfcompare/deploys/670d311b6d980200081d6929
Deploy Preview https://deploy-preview-774--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.

syedbarimanjan commented 5 days ago

Make the pr description better like adding pictures and explaining your implementaion.

Also you should look into the CI tests which are failing, read this section of the project readme which will help you with it.

hibaa03 commented 5 days ago

Hi, could you add the screenshots to the description for more clarification? Also run the tests and validations locally to see which ones are failing and work on the issues as you will see on your terminal. Maybe its due to jest snapshot which you will have to update

FatumaA commented 4 days ago

Oops forgot to update the description from when it was on draft mode

FatumaA commented 3 days ago

Thanks, this looks pretty good as a first draft!

I left a comment about the mechanism to change the search params.

Otherwise I think that it should be make editable after a click on a button.

So here is how it could look like:

  1. at load time, we still have the "Results" by default (or the value in the URL if present) => the good thing is that most of the tests would still work.
  2. add a little edit icon (with MUI's Edit icon) next to this text. => i'd put it on the right of the text, this would look like : Results [Edit icon] | Compare with a base
  3. when the user clicks the edit icon, then the text changes into an input like you did.

It would be good to extract this functionality into a separate react component.

Finally it would be good to add a test to test this functionality.

Thanks again!

Thanks for the review. Working on it 🙂.

FatumaA commented 18 hours ago

Thanks, this looks pretty good as a first draft!

I left a comment about the mechanism to change the search params.

Otherwise I think that it should be make editable after a click on a button.

So here is how it could look like:

  1. at load time, we still have the "Results" by default (or the value in the URL if present) => the good thing is that most of the tests would still work.
  2. add a little edit icon (with MUI's Edit icon) next to this text. => i'd put it on the right of the text, this would look like : Results [Edit icon] | Compare with a base
  3. when the user clicks the edit icon, then the text changes into an input like you did.

It would be good to extract this functionality into a separate react component.

Finally it would be good to add a test to test this functionality.

Thanks again!

Hi, I think this is ok now. Except for the tests. I've commented out the last 2 tests because they keep timing out and I'm not sure how to go about that.

I'd appreciate a look there, thank you 🙏🏽

hibaa03 commented 18 hours ago

Make sure your internet connection isn't slow and you release some memory of your system if needed. And run the tests using npm test -- --testTimeout 60000 if they're still timing out.

FatumaA commented 16 hours ago

Make sure your internet connection isn't slow and you release some memory of your system if needed. And run the tests using npm test -- --testTimeout 60000 if they're still timing out.

Could be slow internet, I did increase timeout and same result