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.

Sign up for GitHub

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

Jump to bottom

Implemented getColumnNames getter #292

Merged
merged 4 commits into from
Jan 5, 2023
Merged

Implemented getColumnNames getter #292

merged 4 commits into from
Jan 5, 2023

Conversation

rmanaem
Copy link
Contributor

@rmanaem rmanaem commented Dec 23, 2022

Closes #255

@rmanaem rmanaem requested a review from surchs December 23, 2022 04:28
@rmanaem rmanaem self-assigned this Dec 23, 2022
surchs
surchs requested changes Jan 3, 2023
View reviewed changes
Copy link
Contributor

@surchs surchs left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @rmanaem. This all looks very good. However #291 has implemented essentially the same getter (minus the tests and component fixes). So I think we should have a chat with @jarmoza on how to proceed with this.

My sense is that we can probably merge #291 without implementing this getter, but for the moment I would wait for us to chat :).

cypress/component/column-linking-table.cy.js Outdated Show resolved Hide resolved
cypress/unit/store-getter-getColumnNames.cy.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
store/index-refactor.js Show resolved Hide resolved
@rmanaem
Copy link
Contributor Author

rmanaem commented Jan 4, 2023 •
edited
Loading

Thanks for the PR @rmanaem. This all looks very good. However #291 has implemented essentially the same getter (minus the tests and component fixes). So I think we should have a chat with @jarmoza on how to proceed with this.

Yea, I saw Jonathan's PR and comment on #255 when working on this so I replaced my implementation with his to avoid unnecessary conflicts. If I recall correctly he also wrote a test for the getColumnNames getter.

My sense is that we can probably merge #291 without implementing this getter, but for the moment I would wait for us to chat :).

I also think we should remove(or move) the getColumnNames test from the createColumnToCategoryMap unit test, but let's discuss both of these issues with Jonathan.

@jarmoza
Copy link
Contributor

jarmoza commented Jan 4, 2023

@rmanaem I agree and will remove the getColumnNames test from my PR.

@jarmoza jarmoza mentioned this pull request Jan 4, 2023
Merged
surchs
surchs approved these changes Jan 4, 2023
View reviewed changes
Copy link
Contributor

@surchs surchs left a comment

Choose a reason for hiding this comment

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

🎉 good to go! 👀

@rmanaem
Implemented getColumnNames getter in refactored store
ce8ca71
@rmanaem
Implemented unit test for getColumnNames getter
bda2426 
Also added "lint-fix" command to package.json file
@rmanaem
Updated components dependent on getColumnNames
830989b
@rmanaem
Added suggested changes from PR review
c58e68a 
- Removed lint-fix and added back husky install that was deleted by accident in package.json file
- Removed the left over debug statement from column-linking-table.cy.js
- Fixed the indentation in store-getter-getColumnNames.cy.js
@rmanaem rmanaem merged commit bd1868d into dev_components_talk_to_store_directly Jan 5, 2023
@rmanaem rmanaem deleted the maint-255 branch February 3, 2023 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@surchs surchs surchs approved these changes

Assignees

@rmanaem rmanaem

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rmanaem @jarmoza @surchs