Make results table title editable #848
Conversation
✅ Deploy Preview for mozilla-perfcompare ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #848 +/- ##
==========================================
+ Coverage 93.56% 93.67% +0.10%
==========================================
Files 94 95 +1
Lines 2643 2704 +61
Branches 526 535 +9
==========================================
+ Hits 2473 2533 +60
- Misses 152 153 +1
Partials 18 18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
thanks for working on it!
Otherwise I'm not sure if the title should be here or rather at the "Results" title part (or next to the results part). But also I think it's pretty fine and I don't have a real good opinion, so if you prefer it there I'm fine with it too. Thanks again! |
Thanks for the review! My last two commits address concerns 3-5. Cancel button behaves as intended with corresponding test. And I stored the previous text as a value, not placeholder. I agree about moving the edit title feature down to the table. However, I'd like to keep the text "Edit title" to stay consistent with our editing UI re 'Edit entry' I'll handle the URL and enter key presses in future commits. With that being said, would you like to see the 'save' button removed or should we allow for both to save the new title in the URL? I think both is better. |
If it doesn't stay at the top, I think "Edit title" would be fine. |
|
I didn't have the time to look at the code, but from the deploy preview I like it much more than before, good work!. A couple comments:
Now or as a follow-up:
As a follow-up, once the implementation has been approved as well:
|
5214ee0 to
6ced3cb
Compare
|
Ahhhh, @julienw, unfortunately I rebased then squashed my commits. I apologize. Hopefully, it's not too hard to review. :-/ |
bedfafc to
a6051c3
Compare
|
#Now or as a follow-up: As a follow-up, once the implementation has been approved as well: I'll take care of the above in follow-up PRs, @julienw |
a6051c3 to
5c5f5dc
Compare
There was a problem hiding this comment.
Thanks, the behavior looks better!
I left a few small comments, tell me what you think.
Especially I don't understand the dance with the slug.
Also, I still don't see this:
when starting the edition with the button, can you make it so that the text is fully selected and the input is focused? You should be able to do it with useEffect and a reference to the input (
input.focus(); input.select();).
@julienw Doesn't this line solve making sure the input is focused and the text is selected? https://github.com/Carla-Moz/perfcompare/blob/3e5025b4e8747221bd4ddd1b8cf1fb3cb0396ba5/src/components/CompareResults/EditTitleInput.tsx#L67 |
This makes it so that the input is selected when it's focused (and it's good!), but not that it's focused when the user presses "edit". This makes that the user needs an extra click to edit the title, which we can avoid. |
Not to be annoying, why is it necessary to have the input focused when the user presses the |
|
Think aboug the whole process of editing the title: Note that I'm not saying to remove your "select on focus" behavior ! Only to add a "focus on edit" behavior. Is there a reason you don't like it? I'd like to understand :-) Thanks |
I suppose. I'll go ahead and add the focus on edit button click but I'd like to talk about it when you return lol. |
Need to add logic to save title name in url
also improved name of editTitleInput Increased width of the header title
Also changed the title names to comparisonTitle to no be confused with page title Increased width of title too
correctly updated snapshots moved edit title to result table header and removed edit references in other views fix cancel button and add corresponding test This makes sure the previous state is stored and shown when the user hits the cancel button moved save and cancel buttons Also added form to handle submissions (save and enter key press) - Handled esc key for cancel - fixed truncated last letter bug for onChange -moved error state to edit title input comp -next need to add tests for error cases and title save refactored EditTitleInput and added remaining test for url update Added PR notes and will remove once approved
256af13 to
6bff0f3
Compare
* added the edit icon beside results page title * added edit title input component Need to add logic to save title name in url * add editable title to other results page views * added save button to editable view also improved name of editTitleInput Increased width of the header title * added logic to save title to url and update title in ui Also changed the title names to comparisonTitle to no be confused with page title Increased width of title too * rebased and updated correctly updated snapshots moved edit title to result table header and removed edit references in other views fix cancel button and add corresponding test This makes sure the previous state is stored and shown when the user hits the cancel button moved save and cancel buttons Also added form to handle submissions (save and enter key press) - Handled esc key for cancel - fixed truncated last letter bug for onChange -moved error state to edit title input comp -next need to add tests for error cases and title save refactored EditTitleInput and added remaining test for url update Added PR notes and will remove once approved * add focus to input and fix header img * resolved ResultsMain comments * resolved EditTitleInput comments * add coverage for error case * add focus and select ref on edit button click * resolved latest comments
* added the edit icon beside results page title * added edit title input component Need to add logic to save title name in url * add editable title to other results page views * added save button to editable view also improved name of editTitleInput Increased width of the header title * added logic to save title to url and update title in ui Also changed the title names to comparisonTitle to no be confused with page title Increased width of title too * rebased and updated correctly updated snapshots moved edit title to result table header and removed edit references in other views fix cancel button and add corresponding test This makes sure the previous state is stored and shown when the user hits the cancel button moved save and cancel buttons Also added form to handle submissions (save and enter key press) - Handled esc key for cancel - fixed truncated last letter bug for onChange -moved error state to edit title input comp -next need to add tests for error cases and title save refactored EditTitleInput and added remaining test for url update Added PR notes and will remove once approved * add focus to input and fix header img * resolved ResultsMain comments * resolved EditTitleInput comments * add coverage for error case * add focus and select ref on edit button click * resolved latest comments
Closes issues Allow users to change the page title of PerfCompare and have the title persist in the url
Deploy preview
UPDATED. Please read before reviewing. The last commit replaces the
forminEditTitleInputwithFormfromreact-router-domand adds a new method ononSaveSubmitto better handle the form submission and error case.In response to first review, please check the following commits:
The following below are handled but this last commit:
EditTitleInputUpdated preview shots:
Outdated: This is split into 5 commits atm to make it easier to review. I'd like a review now before adding the logic to the other Results View pages
Note: regarding my last commit, I forgot to put the
titleMessageErrorbelow the input. Will do in next commit.