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

front: fix none display when no study type in operational studies #6746

Merged

Conversation

RomainValls
Copy link
Contributor

closes #6655

@RomainValls RomainValls requested a review from a team as a code owner February 26, 2024 14:02
Copy link

codecov bot commented Feb 26, 2024

Codecov Report

Attention: Patch coverage is 0% with 25 lines in your changes are missing coverage. Please review.

Project coverage is 28.65%. Comparing base (cbe74a9) to head (7fad5bf).

Files Patch % Lines
...ront/src/applications/operationalStudies/consts.ts 0.00% 13 Missing ⚠️
...c/modules/study/components/AddOrEditStudyModal.tsx 0.00% 10 Missing ⚠️
front/src/modules/study/utils.ts 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##                dev    #6746      +/-   ##
============================================
- Coverage     28.65%   28.65%   -0.01%     
  Complexity     2235     2235              
============================================
  Files          1063     1063              
  Lines        131643   131643              
  Branches       2647     2647              
============================================
- Hits          37723    37722       -1     
- Misses        92399    92400       +1     
  Partials       1521     1521              
Flag Coverage Δ
core 79.78% <ø> (ø)
editoast 74.90% <ø> (-0.01%) ⬇️
front 8.92% <0.00%> (ø)
gateway 2.49% <ø> (ø)
railjson_generator 87.26% <ø> (ø)
tests 83.07% <ø> (ø)

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.

@RomainValls RomainValls force-pushed the rvs/front-fix-none-display-on-studycard-in-op-studies branch from 0adfff1 to 85eca69 Compare February 26, 2024 14:06
@RomainValls RomainValls changed the title fix none display when no study type in operational studies front: fix none display when no study type in operational studies Feb 26, 2024
Copy link
Contributor

@Akctarus Akctarus left a comment

Choose a reason for hiding this comment

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

Tested, LGTM :)

@RomainValls RomainValls self-assigned this Feb 27, 2024
Copy link
Contributor

@SharglutDev SharglutDev left a comment

Choose a reason for hiding this comment

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

You don't need to change anything in AddOrEditStudyModal.tsx. To solve the issue, you can just hide the study_type div if study_type === 'nothingSelected' in StudyCard.tsx and Study.tsx

Edit : as discussed with @RomainValls and @clarani, we are going to handle the issue by putting an empty string for the "none" study_type and see with the back-end if they can avoid letting the front send undefined or null for the study_type.

Copy link
Contributor

@SharglutDev SharglutDev left a comment

Choose a reason for hiding this comment

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

Thank you for the changes, everything works great. Just one last comment for me.

@RomainValls RomainValls force-pushed the rvs/front-fix-none-display-on-studycard-in-op-studies branch 3 times, most recently from 72cf6c3 to f3e6604 Compare March 11, 2024 16:48
Copy link
Contributor

@SharglutDev SharglutDev left a comment

Choose a reason for hiding this comment

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

Lgtm, good job !

To go further, I think we could, in another PR, do the same logic for the study_states by putting them in an array with as const and maybe we could reuse the createSelectOptions function for both states and types.

What do you think @clarani ?

Copy link
Contributor

@clarani clarani left a comment

Choose a reason for hiding this comment

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

✅ LGTM, only a small comment :)

@clarani
Copy link
Contributor

clarani commented Mar 14, 2024

Yes agree with @SharglutDev for studyStates, we should create an issue for that 🙏

@RomainValls RomainValls force-pushed the rvs/front-fix-none-display-on-studycard-in-op-studies branch from f3e6604 to 0a90248 Compare March 14, 2024 16:18
@RomainValls RomainValls force-pushed the rvs/front-fix-none-display-on-studycard-in-op-studies branch from 0a90248 to 7fad5bf Compare March 14, 2024 16:27
@RomainValls RomainValls added this pull request to the merge queue Mar 14, 2024
Merged via the queue into dev with commit d8e883a Mar 14, 2024
22 checks passed
@RomainValls RomainValls deleted the rvs/front-fix-none-display-on-studycard-in-op-studies branch March 14, 2024 16:43
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.

__None__ displayed in study description when Type = __None__
4 participants