5413 - [Notebook] Various visual issues with renaming sections/pages

[mariuszr]

Closes #5413

Describe your changes:

1. Section / Page names are not truncated while input is active.
2. Hitting Enter does confirm the rename and does not add a newline (no multiple line option)
3. After completely removing a section or page title and entering or blurring action returns the default value 'Unnamed Section/Page'
4. Separate selectability and editability actions for Section and Page components
5. Additionally, added the Escape option that returns the initial value after hitting Esc

All Submissions:

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?
• Is this change backwards compatible? For example, developers won't need to change how they are calling the API or how they've extended core plugins such as Tables or Plots.

Author Checklist

• Changes address original issue?
• Unit tests included and/or updated with changes?
• Command line build passes?
• Has this been smoke tested?
• Testing instructions included in associated issue OR is this a dependency/testcase change?

Reviewer Checklist

• Changes appear to address issue?
• Changes appear not to be breaking changes?
• Appropriate unit tests included?
• Code style and in-line documentation are appropriate?
• Commit messages meet standards?
• Has associated issue been labelled unverified? (only applicable if this PR closes the issue)
• Has associated issue been labelled bug? (only applicable if this PR is for a bug fix)

[codecov]

Codecov Report

Merging #5475 (ff51127) into master (ca92837) will decrease coverage by 0.00%.
The diff coverage is 5.88%.

❗ Current head ff51127 differs from pull request most recent head b0b2d94. Consider uploading reports for the commit b0b2d94 to get more accurate results

@@            Coverage Diff             @@
##           master    #5475      +/-   ##
==========================================
- Coverage   53.34%   53.33%   -0.01%     
==========================================
  Files         594      595       +1     
  Lines       22016    22018       +2     
  Branches     2051     2055       +4     
==========================================
- Hits        11745    11744       -1     
- Misses       9698     9700       +2     
- Partials      573      574       +1     

Flag	Coverage Δ		*Carryforward flag
e2e-ci	39.49% <ø> (ø)		Carriedforward from ca92837
e2e-full	48.44% <ø> (ø)		Carriedforward from ca92837
e2e-stable	47.50% <100.00%> (+<0.01%)	⬆️
unit	50.13% <5.88%> (-0.01%)	⬇️

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files	Coverage Δ
src/plugins/notebook/components/PageComponent.vue	15.62% <0.00%> (ø)
...c/plugins/notebook/components/SectionComponent.vue	15.62% <0.00%> (ø)
src/plugins/notebook/utils/notebook-key-code.js	100.00% <100.00%> (ø)
.../plugins/timeConductor/ConductorInputsRealtime.vue	49.39% <0.00%> (-1.21%)	⬇️
src/plugins/gauge/components/Gauge.vue	61.72% <0.00%> (-0.62%)	⬇️
...c/plugins/persistence/couch/CouchObjectProvider.js	57.52% <0.00%> (-0.25%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca92837...b0b2d94. Read the comment docs.

[mariuszr]

Hey @jvigliotta and @unlikelyzero, Please review

[unlikelyzero]

@mariuszr great work! We're headsdown on a major release and will have this totally reviewed by the end of the week! In the meantime, would you mind adding coverage for some of this functionality so that it doesn't regress in the future?

For the keystroke / key input commands, I think they'd best be suited as e2e tests.

[unlikelyzero]

@mariuszr can you rebase now that we've merged 2.0.5 back into master?

[mariuszr]

@unlikelyzero Merged with the master's source code.

I will be able to take a look at the e2e in the next week.

[ozyx]

Looking good! Requesting a couple of small changes.

[ozyx]

Hi @mariuszr ! Sorry for the delay. I've left a couple suggestions, but overall this is looking really good. The next step would be to cover these changes with some e2e tests. We've recently refactored our e2e framework and folder structure, and added a bunch of documentation. Please check it out and let us know if you have any questions or run into any issues while working on the e2e tests. We are happy to provide any guidance if needed as well. Thanks!

[mariuszr]

Hi @ozyx, I'll take a look at the e2e tests.

[charlesh88]

@mariuszr This is fantastic, thanks so much! There's an additional regression issue that we found that in master that isn't yet addressed, and I'm hoping you might be able to add a fix to this PR for it. I've added the note to the original issue, but to recap here:

Clicking on a section or page to navigate to it immediately puts the name into edit mode, but doesn't assign the correct CSS class for the contenteditable span element. This makes using the nav pane to view pages a very bad experience, and adds a bunch of risk that a section or page may be accidentally renamed.

Instead, the user should only be able to edit a previously selected section or page. So to rename a non-selected section or page, they must select it, then click it again.

This mimics how file renaming works in many computer operating systems. This is how it used to work and how it should work now.

If you can't address this particular problem in this PR, let us know and we'll merge this in and file a separate issue. But if you can, that'd be even more appreciated. In either case, thanks so much for the contributions, effort and interest.

[mariuszr]

Hi @charlesh88, Please note that the plugin header name also behaves with one click option to get the header editable.

Providing the double click option to make the component value editable may cause inconsistency in that type of interaction.

[charlesh88]

Hi @charlesh88, Please note that the plugin header name also behaves with one click option to get the header editable.

That is true, but there's an important difference: the main view header name is not really "selectable". The Notebook navigation is more akin to a list of files in a folder view; "selecting" a page or section primarily navigates to that location, in the same way that items in the tree navigate to their views when clicked. The problem we're solving for in the Notebook nav is to also allow a secondary rename action.

[mariuszr]

That is true, but there's an important difference: the main view header name is not really "selectable". The Notebook navigation is more akin to a list of files in a folder view; "selecting" a page or section primarily navigates to that location, in the same way that items in the tree navigate to their views when clicked. The problem we're solving for in the Notebook nav is to also allow a secondary rename action.

@charlesh88 I got your point. Thank you for the explanation.

I have separated the selectability and editability actions for the Section and Page components.

[mariuszr]

Hi @unlikelyzero and @ozyx Can we create a new issue for creating the E2E test for the task? I will be able to provide it at a later time.

[ozyx]

Hi @mariuszr,

Sure! Not a problem, but would you mind adding a couple test stubs so that we don't forget to add coverage here? You can just follow the same pattern established here. No need to implement the tests, just a couple test.fixme stubs with some comments describing the test procedures.

Other than that, this PR is just pending a final review from @charlesh88 before we can merge.

Thanks again for all your work on this!

[charlesh88]

This looks really good, thanks for the changes. I pushed up a very minor tweak that fixes the layout of the list items in the Notebook nav pane to align the affordance arrows:

With that, this LGTM. Thanks again!