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

fix: make textValue default to key for Normalized Item #2113

Merged
merged 5 commits into from
Jun 28, 2024

Conversation

AkshatJawne
Copy link
Contributor

Resolves #2112

@AkshatJawne AkshatJawne self-assigned this Jun 27, 2024
@bmingles bmingles self-requested a review June 27, 2024 18:15
Copy link
Contributor

@bmingles bmingles left a comment

Choose a reason for hiding this comment

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

Left comments

@@ -105,7 +105,7 @@ describe.each([
it.each([
[
{ key: 'mock.key', textValue: undefined },
'Empty',
'mock.key',
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor test coverage regression:

Before:
image

Afer (lines 55,104 missing coverage):
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a test case for { key: '', textValue: undefined } resulting in Empty ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have one for key being undefined and textvalue being undefined resulting in Empty, but not for key as an empty string, can add that as well if you would like: currently coverage is 100% though.

@AkshatJawne
Copy link
Contributor Author

Ready for re-review

Copy link

codecov bot commented Jun 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.64%. Comparing base (e13c9d7) to head (41fb5cb).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2113      +/-   ##
==========================================
+ Coverage   46.60%   46.64%   +0.04%     
==========================================
  Files         679      679              
  Lines       38598    38634      +36     
  Branches     9779     9794      +15     
==========================================
+ Hits        17988    18022      +34     
- Misses      20558    20560       +2     
  Partials       52       52              
Flag Coverage Δ
unit 46.64% <100.00%> (+0.04%) ⬆️

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.

Copy link
Contributor

@bmingles bmingles left a comment

Choose a reason for hiding this comment

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

Changes look good. Verify test coverage is still 100%

@AkshatJawne AkshatJawne merged commit bd3e944 into deephaven:main Jun 28, 2024
11 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jun 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make text_value prop default to key for Item component
2 participants