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

chore(Cross): [IOAPPX-370] Enable discrete transition in the BonusCardScreenComponent #6171

Merged

Conversation

dmnplb
Copy link
Contributor

@dmnplb dmnplb commented Sep 11, 2024

Short description

This PR enables discrete transition in the BonusCardScreenComponent thanks to the new updated useHeaderSecondLevel hook.

List of changes proposed in this pull request

  • Add support for enableDiscreteTransition to the BonusCardScreenComponent
  • Partially refactor CgnDetailScreen to improve compatibility with the IOScrollView (cc @freddi301 @Hantex9)
  • Add dark mode support to the BonusCardShape component
  • Add some documentation to the IOScrollView component

Preview

updatedCgnDetailScreen.mp4

How to test

  1. Locally launch the app
  2. Go to the DS → Bonus Card Screen or go the CGN main screen

@dmnplb dmnplb added the Design System New visual language and reduction of previous UI clutter label Sep 11, 2024
@dmnplb dmnplb requested a review from a team as a code owner September 11, 2024 13:07
@pagopa-github-bot pagopa-github-bot changed the title [IOAPPX-370] Enable discrete transition in the BonusCardScreenComponent chore(Cross): [IOAPPX-370] Enable discrete transition in the BonusCardScreenComponent Sep 11, 2024
@pagopa-github-bot
Copy link
Collaborator

pagopa-github-bot commented Sep 11, 2024

Affected stories

  • ⚙️ IOAPPX-370: Aggiunta della transizione discreta a BonusCardScreenComponent
    subtask of

Generated by 🚫 dangerJS against b20dc26

@freddi301
Copy link
Collaborator

I have some questions. Feel free to not answer.

  1. Why do you use useSecondLevelHeader hook instead of headerConfig prop? (I removed the hook in favor of the prop in a previous PR)
  2. I couldn't find any uses of these props across the codebase: faqCategories, contextualHelpMarkdown, contextualHelp. Are they planned for future use?

Copy link

codecov bot commented Sep 11, 2024

Codecov Report

Attention: Patch coverage is 38.88889% with 11 lines in your changes missing coverage. Please review.

Project coverage is 47.94%. Comparing base (4f204b4) to head (b20dc26).
Report is 511 commits behind head on master.

Files with missing lines Patch % Lines
ts/features/bonus/cgn/screens/CgnDetailScreen.tsx 0.00% 10 Missing ⚠️
.../features/design-system/core/DSBonusCardScreen.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6171      +/-   ##
==========================================
- Coverage   48.42%   47.94%   -0.48%     
==========================================
  Files        1488     1761     +273     
  Lines       31617    35673    +4056     
  Branches     7669     8449     +780     
==========================================
+ Hits        15311    17104    +1793     
- Misses      16238    18510    +2272     
+ Partials       68       59       -9     
Files with missing lines Coverage Δ
.../components/BonusCard/BonusCardScreenComponent.tsx 100.00% <100.00%> (+16.12%) ⬆️
ts/components/BonusCard/BonusCardShape.tsx 81.81% <100.00%> (-18.19%) ⬇️
ts/components/ui/IOScrollView.tsx 85.91% <ø> (ø)
...y/details/screens/IdPayInitiativeDetailsScreen.tsx 83.33% <100.00%> (+6.62%) ⬆️
.../features/design-system/core/DSBonusCardScreen.tsx 20.00% <0.00%> (ø)
ts/features/bonus/cgn/screens/CgnDetailScreen.tsx 4.54% <0.00%> (-9.41%) ⬇️

... and 1257 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 529ea6c...b20dc26. Read the comment docs.

@dmnplb
Copy link
Contributor Author

dmnplb commented Sep 11, 2024

@freddi301 Answers below:

Why do you use useSecondLevelHeader hook instead of headerConfig prop? (I removed the hook in favor of the prop in a previous PR)

In short: I've left headerConfig to give developers the maximum freedom in header configuration, if needed. In this particular case, you have simply re-declared some values already managed by the useHeaderSecondLevel hook (as fallback values). I preferred to use the hook directly to avoid code duplication. Both methods use useLayoutEffect internally, anyway.

I couldn't find any uses of these props across the codebase: faqCategories, contextualHelpMarkdown, contextualHelp. Are they planned for future use?

They are props related to the Help section. They're actually managed by the header, but they probably need a refactor/refresh.

@freddi301
Copy link
Collaborator

@freddi301 Answers below:

Why do you use useSecondLevelHeader hook instead of headerConfig prop? (I removed the hook in favor of the prop in a previous PR)

In short: I've left headerConfig to give developers the maximum freedom in header configuration, if needed. In this particular case, you have simply re-declared some values already managed by the useHeaderSecondLevel hook (as fallback values). I preferred to use the hook directly to avoid code duplication. Both methods use useLayoutEffect internally, anyway.

I couldn't find any uses of these props across the codebase: faqCategories, contextualHelpMarkdown, contextualHelp. Are they planned for future use?

They are props related to the Help section. They're actually managed by the header, but they probably need a refactor/refresh.

I think that having two opinionated ways of doing the same thing might be confusing for new entries. Maybe we can leave a jsdoc comment containing exactly what you answered me.

I didn't find any code that uses those help related props.

Feel absolutely free to ignore these suggestions

@dmnplb
Copy link
Contributor Author

dmnplb commented Sep 12, 2024

@freddi301 That's a good suggestion. Added some documentation in the following commit → 95a9707

@dmnplb dmnplb added the code review needed 💻📱 Insert this label if you need a code review that no one in your team can give you label Sep 19, 2024
Copy link
Contributor

@Hantex9 Hantex9 left a comment

Choose a reason for hiding this comment

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

LGTM!

@dmnplb dmnplb merged commit 523ee40 into master Sep 23, 2024
13 checks passed
@dmnplb dmnplb deleted the IOAPPX-370-update-bonuscardscreencomponent-discrete-transition branch September 23, 2024 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code review needed 💻📱 Insert this label if you need a code review that no one in your team can give you Cross Design System New visual language and reduction of previous UI clutter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants