Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Capitalized references of description in store and updated affected unit tests #391

Merged
merged 1 commit into from
Apr 4, 2023

Conversation

rmanaem
Copy link
Contributor

@rmanaem rmanaem commented Apr 3, 2023

This PR includes the fix for the bug outlined in #384.
Changes include changing description references to Description in

  • index.js
  • store-mutation-initializeDataDictionary.cy.js
  • store-getter-getColumDescription.cy.js

Closes #384

@jarmoza jarmoza self-assigned this Apr 4, 2023
@jarmoza jarmoza self-requested a review April 4, 2023 18:13
Copy link
Contributor

@jarmoza jarmoza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rmanaem Can you also write a test for the below condition from #384 ?

The component test for the column-linking-table should fail if 1) description text exists for a data table column and 2) it is not visible in the 'description' component column

So in other words, the failure should be a passing test.

@rmanaem
Copy link
Contributor Author

rmanaem commented Apr 4, 2023

@jarmoza I'm not quite sure what that test would look like since there already is a test checking if the column descriptions are correctly displayed in column-linking-table.cy.js.
I think the original description of the #384 issue was written under the assumption that the bug was coming from column-linking-table component however, the bug was found in the store.

Perhaps a test with mock data having description as their property instead of Description and expecting the description not to be displayed?

@jarmoza
Copy link
Contributor

jarmoza commented Apr 4, 2023

Perhaps a test with mock data having description as their property instead of Description and expecting the description not to be displayed?

@rmanaem That would work given our current code. If we do some kind of forced capitalization later on, we can readdress.

Copy link
Contributor

@jarmoza jarmoza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In light of our discussion re: a new feature to enforce capitalization (see #316, #393), we shouldn't need the test above.

So, 👨‍🍳

@rmanaem rmanaem merged commit 39c1667 into dev_components_talk_to_store_directly Apr 4, 2023
@rmanaem rmanaem deleted the bug-384 branch April 4, 2023 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants