feat: add overflowTooltip to editableLabel

[mathieu-anderson]

Resolves: #543

Description

The objective was to add a tooltip to column names when their content overflows. This PR adds overflowTooltip({placement: 'top'}) to editableLabel in koForm, which achieves this results:

Screen.Recording.2023-11-03.at.10.32.47.mov

Questions

I opened this as draft because of testing.

I noticed there was no test case for editableLabel, I added one following what was present for the other koForm entities. However, i am not sure how to test the rendering of a tooltip.

I assume that I need to trigger a mouseenter event and then select the test classname 'info-tooltip-popup' from the DOM, but I'm not sure how to do this.

I'm also unsure if we want to unit test this, or if it should be tested in the end-to-end tests.

[mathieu-anderson]

Will look at this failing test locally 🤔

[mathieu-anderson]

A little bit confused because calling env GREP_TESTS=dispose yarn test as suggested here does not seem to run the test in dispose.js which is failing on CI 🤔

[georgegevoian]

Resolves: #543

Description

The objective was to add a tooltip to column names when their content overflows. This PR adds overflowTooltip({placement: 'top'}) to editableLabel in koForm, which achieves this results:

Screen.Recording.2023-11-03.at.10.32.47.mov

Questions

I opened this as draft because of testing.

I noticed there was no test case for editableLabel, I added one following what was present for the other koForm entities. However, i am not sure how to test the rendering of a tooltip.

I assume that I need to trigger a mouseenter event and then select the test classname 'info-tooltip-popup' from the DOM, but I'm not sure how to do this.

I'm also unsure if this is where we want to test this, or if it should be etsted in the end-to-end tests.

Thanks for opening the PR @mathieu-anderson.

The code in koForm.js is quite old and I'm not sure we'll ever reference it again in newer code. We've been moving away from using knockout in favor of grainjs, which is our own library that's similar to knockout. For that reason, I think it's ok if you skip adding a test for it in koForm.js.

I'm fairly certain column names are the only UI component in Grist that use it, so it should suffice to add an end-to-end test that changes a column name to be long enough to overflow, moves the mouse over the name, and checks that a tooltip appears with the full column name inside. Our end-to-end tests are stored in /test/nbrowser/ and use a library called mocha-webdriver, which is a thin wrapper over selenium-webdriver. You can create a new test file, or add a test to an existing file (perhaps GridView.ts, although there may be another that's a better fit). Something like the following should work, although I haven't tried running it myself, so there may be a tweak needed here or there to get it passing:

  it('should show a tooltip for overly long column names', async function() {
    const columnName = 'AReallyReallyReallyLongColumnName';
    await gu.addColumn(columnName);
    await getColumnHeader({col: columnName}).mouseMove();
    await driver.wait(() => driver.findWait('.test-tooltip', 500).isDisplayed(), 3000);
    assert.equal(await driver.find('.test-tooltip').getText(), columnName);
  });

[mathieu-anderson]

Thank you, will update the PR later today!

[mathieu-anderson]

Hello! Sorry for not updating as I said, I got sick and then lost the time ^^'

I tried adding a test case to GridView.ts as suggested. I only used the suggested code as a first pass, to see the test run. I ran env GREP_TESTS=GridView yarn test, expecting to be able to run these tests in isolation (it's very long to run the full e2e test suite), but somehow my new test case did not run 🤔

I pushed the commits so that it's more clearly visible.

[mathieu-anderson]

The test added did pass on CI though: https://github.com/gristlabs/grist-core/actions/runs/6841340038/job/18601578573?pr=720#step:17:427

However, this one is failing, will look at it: https://github.com/gristlabs/grist-core/actions/runs/6841340038/job/18601578573?pr=720#step:17:494

[georgegevoian]

Hello! Sorry for not updating as I said, I got sick and then lost the time ^^'

I tried adding a test case to GridView.ts as suggested. I only used the suggested code as a first pass, to see the test run. I ran env GREP_TESTS=GridView yarn test, expecting to be able to run these tests in isolation (it's very long to run the full e2e test suite), but somehow my new test case did not run 🤔

I pushed the commits so that it's more clearly visible.

Strange. Could it be that you weren't running grist-core when you created the test? A rebuild should help with that, though normally there's a watcher while grist-core is running that handles changes to files. Glad to see it passing in CI.

[georgegevoian]

The test added did pass on CI though: https://github.com/gristlabs/grist-core/actions/runs/6841340038/job/18601578573?pr=720#step:17:427

However, this one is failing, will look at it: https://github.com/gristlabs/grist-core/actions/runs/6841340038/job/18601578573?pr=720#step:17:494

I'll have some time later this week to look into the failure. Seems like maybe something isn't getting cleaned up in editableLabel.