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

consistency around non required fields #161

Merged
merged 1 commit into from
Feb 28, 2024
Merged

Conversation

meganrm
Copy link
Contributor

@meganrm meganrm commented Jan 26, 2024

Problem

we want to make sure the schema is consistent with what is truly required to not break the front end. Related to allen-cell-animated/cell-feature-data#92

Solution

Most of these fields were already being treated as non required, but needed to make sure the interface is consistent

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Comment on lines 29 to 35
userData: {
isNew: boolean;
inReview: boolean;
totalTaggedStructures: number;
totalCells: number;
totalFOVs: number;
isNew?: boolean;
inReview?: boolean;
totalTaggedStructures?: number;
totalCells?: number;
totalFOVs?: number;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Should userData also be allowed to be undefined?

Choose a reason for hiding this comment

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

Seems like a valid question for overall strategy, but the && conditionals that look at the properties of userData in DatasetCard will throw errors if you make this optional now
userData.totalTaggedStructures && <Descrip...

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 prefer to not have to check for userData && userData.isNew for example, so I initialized it as an empty object in state. It's not required in the dataset, but it will always be defined at this point in the app

@interim17
Copy link

LGTM, also curious about Peyton's question above, but I know there is limited bandwidth for maintenance on this app and probably best to touch less code.

Copy link
Contributor

@toloudis toloudis left a comment

Choose a reason for hiding this comment

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

LGTM but kinda hard to know if we missed anything without unit tests that have various things missing. Is there a good way to know that we are comprehensively in sync with cell-feature-data?

@meganrm meganrm merged commit 690c2db into main Feb 28, 2024
1 check passed
@meganrm meganrm deleted the feature/non-required-data branch February 28, 2024 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants