188618452 new table clean up #1653

eireland · 2024-11-26T21:24:33Z

On new tables:

Removes "no cases" div
Removes empty case above the input line
Changes default attribute name to "Attribute Name" instead of "AttributeName"
Changes table created from the --new-- in the table menu default width to just show the one default column

Changes case table default width to the width of one attribute

codecov · 2024-11-26T21:24:56Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.73%. Comparing base (8d80b3b) to head (c228b2b).
Report is 82 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1653      +/-   ##
==========================================
+ Coverage   85.71%   85.73%   +0.02%     
==========================================
  Files         603      603              
  Lines       30591    30628      +37     
  Branches     8410     8426      +16     
==========================================
+ Hits        26221    26259      +38     
  Misses       4049     4049              
+ Partials      321      320       -1

Flag	Coverage Δ
cypress	`75.26% <100.00%> (-0.02%)`	⬇️
jest	`52.80% <87.50%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cypress · 2024-11-26T21:27:43Z

codap-v3 Run #5458

Run Properties: Passed #5458 • 6e69e65eca: 188618452 new table clean up (#1653)

Project	`codap-v3`
Branch Review	`main`
Run status	`Passed #5458`
Run duration	`05m 47s`
Commit	`6e69e65eca: 188618452 new table clean up (#1653)`
Committer	`Evangeline Ireland`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`0`
Pending	`36`
Skipped	`0`
Passing	`220`
View all changes introduced in this branch ↗︎

… table

…gging. All other failing tests are fixed.

…ibute name from "AttributeName" to "Attribute Name"

tealefristoe

Overall looks good. I just had a few questions/suggestions that I think are worth addressing.

tealefristoe · 2024-12-03T23:51:43Z

v3/src/components/case-tile-common/case-tile-utils.ts

@@ -62,7 +63,10 @@ export function createTableOrCardForDataset (
  }
  caseMetadata.setLastShownTableOrCardTileId(tile.id)

-  const width = caseTableComponentInfo.defaultWidth || 0
+  const numAttributes = sharedDataSet.dataSet.attributes.length
+  const attributesWidth = Math.min(580, (numAttributes * 80) + kDropzoneWidth + kIndexColumnWidth)


Where do 580 and 80 come from? Seems like these should be named constants.

tealefristoe · 2024-12-03T23:55:10Z

v3/src/components/case-tile-common/case-tile-utils.ts

-  const width = caseTableComponentInfo.defaultWidth || 0
+  const numAttributes = sharedDataSet.dataSet.attributes.length
+  const attributesWidth = Math.min(580, (numAttributes * 80) + kDropzoneWidth + kIndexColumnWidth)
+  const width = Math.min(caseTableComponentInfo.defaultWidth || 0, attributesWidth)


I wonder if this should really use Math.min? If caseTableComponentInfo.defaultWidth is falsy, this will always be 0 (maybe that will never occur?). It also feels strange to have two Math.mins in a row. Could you just combine the two lines and replace 580 with caseTableComponentInfo.defaultWidth?

tealefristoe · 2024-12-03T23:59:46Z

v3/cypress/e2e/plugin.spec.ts

+    // For tests involving drag and drop of components with attribute "Attribute Name",
+    // we use a different drag and drop code because the cy.command version includes a
+    // "contains" expectation of text "Attribute Name", which it cannot find because it is over
+    // two spans. This code omits the "contains" expectation because the selector already has enough
+    // information without needing to find a specific text in a span


Would it be possible to change the existing drag function to handle an attribute with a name that is in two spans? If not, I think it would at least be good to create a function to do this special drag, rather than repeating the code three times.

I guess one higher level question is why does the default name not fit in the default width of a column?

The default column width came from V2.
I couldn't get the existing function to work with the attribute name over 2 spans. I'll modify it and add it to the table element object.

tealefristoe · 2024-12-04T00:04:32Z

v3/cypress/e2e/formula/formula-component.spec.ts

@@ -83,15 +83,16 @@ context("Formula Engine", () => {
        "❌ Undefined symbol v1"
      ])
    })
-    it("Formula in a new dataset", () => {
+    // Skipped for now until insert cases on input row is implemnted


How difficult would it be to implement this? Isn't this already done in table.spec.ts lines 738-744?

Part of this PR removed the single case created when a new table is first created.
Input row index cell does not open the index column menu so new cases cannot be inserted without having to enter data.
The table.spec.ts lines 738-744 tests that a new case is created at the bottom when user enters data into the input line.

I think you should use the code in table.spec.ts to add a case so you have access to the index column menu, then continue the test like before. It shouldn't be necessary to skip this test.

…e names.

kswenson

👍 LGTM aside from the issues @tealefristoe pointed out.

tealefristoe

Looks good, thanks for addressing those issues 👍

eireland added 3 commits November 26, 2024 11:38

removes the no data temporary div

93ee66a

Removes empty case when creating a new table

83f7c55

Adds a space to default Attribute Name

45b6cec

Changes case table default width to the width of one attribute

Fixes smoke test to match changes in new table behavior

621e7a2

eireland temporarily deployed to branches November 26, 2024 21:36 — with GitHub Actions Inactive

removes the empty case that's added when a new dataset is created

111e2e8

eireland temporarily deployed to branches November 26, 2024 21:43 — with GitHub Actions Inactive

eireland added v3 CODAP v3 run regression labels Nov 26, 2024

eireland marked this pull request as ready for review November 27, 2024 20:25

eireland marked this pull request as draft November 27, 2024 20:25

Changes table width to just one attribute's worth when creating a new…

e684b75

… table

eireland temporarily deployed to branches November 27, 2024 20:45 — with GitHub Actions Inactive

eireland added 2 commits December 3, 2024 04:20

plugin spect test not working on Broadcast notifiations involving dra…

b2dd8c3

…gging. All other failing tests are fixed.

Fixes plugin cypress test that got broken when we changed intial attr…

0e974c6

…ibute name from "AttributeName" to "Attribute Name"

eireland temporarily deployed to branches December 3, 2024 22:35 — with GitHub Actions Inactive

eireland marked this pull request as ready for review December 3, 2024 22:49

eireland requested review from kswenson and tealefristoe December 3, 2024 22:49

tealefristoe requested changes Dec 4, 2024

View reviewed changes

eireland added 2 commits December 4, 2024 17:40

Made a new function in TableTileElements for moving two line attribut…

39a29a3

…e names.

Simplifies table width logic

f43bcd2

eireland temporarily deployed to branches December 4, 2024 09:52 — with GitHub Actions Inactive

eireland requested a review from tealefristoe December 4, 2024 11:26

kswenson approved these changes Dec 4, 2024

View reviewed changes

unskips cypress test, and fixes it to work

c228b2b

eireland deployed to branches December 5, 2024 00:40 — with GitHub Actions View deployment

tealefristoe approved these changes Dec 5, 2024

View reviewed changes

eireland merged commit 6e69e65 into main Dec 5, 2024
20 of 21 checks passed

eireland deleted the 188618452-new-table-clean-up branch December 5, 2024 21:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

188618452 new table clean up #1653

188618452 new table clean up #1653

eireland commented Nov 26, 2024 •

edited

Loading

codecov bot commented Nov 26, 2024 •

edited

Loading

cypress bot commented Nov 26, 2024 •

edited

Loading

tealefristoe left a comment

tealefristoe Dec 3, 2024

tealefristoe Dec 3, 2024

tealefristoe Dec 3, 2024

eireland Dec 4, 2024

tealefristoe Dec 4, 2024

eireland Dec 4, 2024

tealefristoe Dec 4, 2024

eireland Dec 5, 2024

kswenson left a comment

tealefristoe left a comment

188618452 new table clean up #1653

188618452 new table clean up #1653

Conversation

eireland commented Nov 26, 2024 • edited Loading

codecov bot commented Nov 26, 2024 • edited Loading

Codecov Report

cypress bot commented Nov 26, 2024 • edited Loading

tealefristoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kswenson left a comment

Choose a reason for hiding this comment

tealefristoe left a comment

Choose a reason for hiding this comment

eireland commented Nov 26, 2024 •

edited

Loading

codecov bot commented Nov 26, 2024 •

edited

Loading

cypress bot commented Nov 26, 2024 •

edited

Loading