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

CDE-60 Lab name and chips counter to be linked to the dataset we are mapping #26

Merged
merged 4 commits into from
Mar 4, 2024

Conversation

Aiga115
Copy link
Contributor

@Aiga115 Aiga115 commented Feb 29, 2024

Issue #CDE-60
Problem: Lab name and chips counter to be linked to the dataset we are mapping
Solution:
Use isRowMapped method to calculate number of mapped data, use total length of datasetMapping to calculate total number of mapped and unmapped data, use the difference between total number of data and number of mapped data to calculate the number of unmapped data, and use getSuggestions method to calculate total number of suggestions available.

Screenshot 2024-02-29 111426

} = useCdeContext();

const [numberOfMapped, setNumberOfMapped] = React.useState(0);
const suggestionsMapping = getSuggestions();
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: @Aiga115 why is this done using a getter function and is the datasetMapping not done using a getter function

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 used getSuggestions function as it was mentioned in a description of a task -> https://metacell.atlassian.net/jira/software/c/projects/CDE/boards/44?selectedIssue=CDE-60
if you follow a link in a description of a task it will get you to function's reference
But is there a getter function to get dataset mapping? I don't think there is @zsinnema

Copy link
Member

Choose a reason for hiding this comment

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

I will look into this one @Aiga115

Copy link
Member

Choose a reason for hiding this comment

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

@zsinnema please have a look at the changes done in commit 1d6e643. Based on our conversation, I decided to split the cde context into 3 different contexts; each with a different purpose:

Data Context: Holds the application's core data;
Service Context: Encapsulates functions and operations that act upon the data;
UI Context: Manages UI-related state and interaction;
For the components that we will create, if they need the data exactly as it is stored, I believe it's fine to use the data context; However, if the component needs to perform some transformation or do some calculations based on the data stored it should use the service context.

const suggestionsMapping = getSuggestions();

const sumOfMappedAndUnmapped = Object.keys(datasetMapping).length;
const numberOfUnmapped = sumOfMappedAndUnmapped - numberOfMapped;
Copy link
Collaborator

Choose a reason for hiding this comment

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

remark: @Aiga115 I would do the computation only at line 76 and remove the variable here

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 see, will fix that @zsinnema

suggestions,
} = useDataContext();

const datasetMappingService = useMemo(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@afonsobspinto why do we need to wrap these functions in a context?

Copy link
Member

Choose a reason for hiding this comment

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

It is not mandatory that we have this service wrapped in a context. It is just a way to have easy access to the data that the services work on.
I like that with the current implementation the components don't need to do useDataContext to get the data to pass for the services but I'm not opposed to change if you prefer otherwise.

@zsinnema zsinnema merged commit 90ade7a into develop Mar 4, 2024
1 check passed
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.

3 participants