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

feat(service-portal): generalize modal #16082

Closed
wants to merge 15 commits into from
Closed

Conversation

thorkellmani
Copy link
Member

@thorkellmani thorkellmani commented Sep 19, 2024

What

  • Fix up service portal <Modal/> component for more customization options
  • Use in health center registration and dentist registration

Why

  • Standardized modal appearance for the entire service portal

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Formatting passes locally with my changes
  • I have rebased against main before asking for a review

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Introduced a new customizable modal component for improved user interaction.
    • Added support for dynamic buttons within the modal for enhanced functionality.
    • Enhanced responsiveness of the modal with adaptive image display based on screen size.
    • Implemented new messaging for dentist registration transfers and health center registration success.
    • Added a selection feature for choosing health center doctors within the registration process.
  • Bug Fixes

    • Improved visibility control of the modal to ensure a smoother user experience during registration processes.
  • Refactor

    • Replaced the old registration modals with a unified modal component for both dentist and health center registrations, streamlining the codebase and improving maintainability.
    • Simplified registration logic to always allow dentist registration.

Copy link
Contributor

coderabbitai bot commented Sep 19, 2024

Walkthrough

This pull request introduces changes to the modal components within the service portal. A new image style is added in the Modal.css.ts file, and the Modal component in Modal.tsx is enhanced with new props for improved configurability. The RegisterModal and its associated files are removed, with the Modal component being integrated into the DentistRegistration and HealthCenterRegistration screens. Additionally, new messaging entries for dentist registration errors and successful health center transfers are introduced.

Changes

File Change Summary
libs/service-portal/core/src/components/Modal/Modal.css.ts Added new style for image element with responsive display properties.
libs/service-portal/core/src/components/Modal/Modal.tsx Updated Props interface to include title, text, buttons, iconSrc, and iconAlt. Added internal state for closing behavior and modified rendering logic.
libs/service-portal/health/src/components/RegisterModal/RegisterModal.css.ts Deleted file containing styles for RegisterModal.
libs/service-portal/health/src/components/RegisterModal/RegisterModal.tsx Deleted RegisterModal component and its associated logic.
libs/service-portal/health/src/components/RegisterModal/index.ts Deleted index file exporting RegisterModal.
libs/service-portal/health/src/lib/messages.ts Added new message for dentist registration errors and modified existing messages for health center registration success.
libs/service-portal/health/src/screens/DentistRegistration/DentistRegistration.tsx Replaced RegisterModal with Modal, added state for modal visibility, and updated error handling.
libs/service-portal/health/src/screens/Dentists/Dentists.tsx Changed canRegister to always return true, simplifying registration logic.
libs/service-portal/health/src/screens/HealthCenterRegistration/HealthCenterRegistration.tsx Replaced RegisterModal with Modal, added state for selected doctor, and modified transfer handling logic.

Possibly related PRs

Suggested labels

automerge

Suggested reviewers

  • thordurhhh
  • disaerna

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@thorkellmani thorkellmani changed the title Feat(service-portal): generalize modal feat(service-portal): generalize modal Sep 19, 2024
Copy link

codecov bot commented Sep 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.86%. Comparing base (8e6c0e3) to head (6235421).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #16082   +/-   ##
=======================================
  Coverage   36.86%   36.86%           
=======================================
  Files        6803     6803           
  Lines      140646   140646           
  Branches    39995    39995           
=======================================
  Hits        51852    51852           
  Misses      88794    88794           
Flag Coverage Δ
air-discount-scheme-web 0.00% <ø> (ø)
api 3.37% <ø> (ø)
application-api-files 57.97% <ø> (ø)
application-core 71.62% <ø> (ø)
application-system-api 41.66% <ø> (ø)
application-template-api-modules 24.28% <ø> (ø)
application-templates-accident-notification 29.44% <ø> (ø)
application-templates-car-recycling 3.12% <ø> (ø)
application-templates-criminal-record 26.63% <ø> (ø)
application-templates-driving-license 18.40% <ø> (ø)
application-templates-estate 12.32% <ø> (ø)
application-templates-example-payment 25.41% <ø> (ø)
application-templates-financial-aid 14.34% <ø> (ø)
application-templates-general-petition 23.68% <ø> (ø)
application-templates-health-insurance 26.62% <ø> (ø)
application-templates-inheritance-report 6.45% <ø> (ø)
application-templates-marriage-conditions 15.23% <ø> (ø)
application-templates-mortgage-certificate 43.96% <ø> (ø)
application-templates-parental-leave 30.13% <ø> (+0.09%) ⬆️
application-types 6.71% <ø> (ø)
application-ui-components 1.28% <ø> (ø)
application-ui-shell 21.27% <ø> (ø)
auth-react 22.77% <ø> (ø)
clients-charge-fjs-v2 24.11% <ø> (ø)
contentful-apps 5.57% <ø> (ø)
financial-aid-backend 56.39% <ø> (ø)
financial-aid-shared 19.03% <ø> (ø)
island-ui-core 28.39% <ø> (ø)
judicial-system-web 27.96% <ø> (ø)
portals-admin-regulations-admin 1.88% <ø> (ø)
portals-core 16.15% <ø> (ø)
services-auth-personal-representative 45.41% <ø> (+0.02%) ⬆️
shared-components 27.65% <ø> (ø)
shared-form-fields 31.59% <ø> (ø)
web 1.83% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...service-portal/core/src/components/Modal/Modal.tsx 14.28% <ø> (ø)
libs/service-portal/health/src/lib/messages.ts 0.00% <ø> (ø)
...creens/DentistRegistration/DentistRegistration.tsx 0.00% <ø> (ø)
...lthCenterRegistration/HealthCenterRegistration.tsx 0.00% <ø> (ø)

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 8e6c0e3...6235421. Read the comment docs.

@datadog-island-is
Copy link

datadog-island-is bot commented Sep 19, 2024

Datadog Report

All test runs 507b4f2 🔗

2 Total Test Services: 0 Failed, 1 Passed
➡️ Test Sessions change in coverage: 21 no change

Test Services
Service Name Failed Known Flaky New Flaky Passed Skipped Total Time Code Coverage Change Test Service View
service-portal-core 0 0 0 5 0 2.01s N/A Link
service-portal-health 0 0 0 0 0 560.25ms 1 no change Link

@thorkellmani thorkellmani added the deploy-feature Deploys features to dev label Oct 1, 2024
Copy link
Contributor

github-actions bot commented Oct 1, 2024

Affected services are: service-portal,system-e2e,
Feature deployment of your services will begin shortly. Your feature will be accessible here:
https://featgeneralize-modal-beta.dev01.devland.is/minarsidur

Deployed services: service-portal.
Excluded services: ``

@thorkellmani thorkellmani marked this pull request as ready for review October 2, 2024 15:40
@thorkellmani thorkellmani requested a review from a team as a code owner October 2, 2024 15:40
Copy link
Member

@thordurhhh thordurhhh left a comment

Choose a reason for hiding this comment

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

fix default true 🚂

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 9

🧹 Outside diff range and nitpick comments (5)
libs/service-portal/core/src/components/Modal/Modal.css.ts (1)

31-39: LGTM! Consider adding comments for clarity.

The new image style is well-implemented and follows responsive design principles. It adheres to the coding guidelines for libs/**/* files by being reusable and using TypeScript.

Suggestions for improvement:

  1. Add a comment explaining the purpose of the image and why it's only shown on larger screens.
  2. Consider using a theme variable for the negative margin to ensure consistency with other components.

You could add a comment like this:

export const image = style({
  // Hide image on small screens and show it on large screens (lg)
  // for improved visual layout in the modal
  display: 'none',
  ...themeUtils.responsiveStyle({
    lg: {
      marginRight: `-${theme.spacing['2']}px`,
      display: 'initial',
    },
  }),
})
libs/service-portal/core/src/components/Modal/Modal.tsx (1)

59-59: Simplify function call using optional chaining

The function call onCloseModal && onCloseModal() can be simplified using optional chaining onCloseModal?.(), which is more concise and idiomatic in TypeScript.

Apply this diff to simplify the function call:

-      onCloseModal && onCloseModal()
+      onCloseModal?.()
🧰 Tools
🪛 Biome

[error] 59-59: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

libs/service-portal/health/src/screens/DentistRegistration/DentistRegistration.tsx (2)

42-42: Nitpick: Remove unnecessary type annotation

The type of modalVisible can be inferred from the initial value false, so the explicit <boolean> type is not needed.

Apply this diff to simplify the code:

- const [modalVisible, setModalVisible] = useState<boolean>(false)
+ const [modalVisible, setModalVisible] = useState(false)

205-207: Improve localization by passing variables to formatMessage

For better localization support, pass the dentist's name as a variable within formatMessage instead of concatenating strings. This ensures correct translations in languages with different grammatical structures.

Apply this change:

- title={`${formatMessage(
-   messages.dentistModalTitle,
- )} ${selectedDentist?.name}`}
+ title={formatMessage(messages.dentistModalTitle, {
+   name: selectedDentist?.name,
+ })}

Update the messages.dentistModalTitle localization string to include a placeholder for the dentist's name, like {name}.

libs/service-portal/health/src/screens/HealthCenterRegistration/HealthCenterRegistration.tsx (1)

305-306: Update placeholder icon to a representative image

The iconSrc is set to './assets/images/coffee.svg', which may be a placeholder. Consider replacing it with an icon that accurately represents the health center registration process to enhance user experience.

If an appropriate icon is not available, you might consider using a relevant icon from the existing asset library or designing a new one.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 81e07f5 and 24f07b5.

📒 Files selected for processing (9)
  • libs/service-portal/core/src/components/Modal/Modal.css.ts (1 hunks)
  • libs/service-portal/core/src/components/Modal/Modal.tsx (5 hunks)
  • libs/service-portal/health/src/components/RegisterModal/RegisterModal.css.ts (0 hunks)
  • libs/service-portal/health/src/components/RegisterModal/RegisterModal.tsx (0 hunks)
  • libs/service-portal/health/src/components/RegisterModal/index.ts (0 hunks)
  • libs/service-portal/health/src/lib/messages.ts (2 hunks)
  • libs/service-portal/health/src/screens/DentistRegistration/DentistRegistration.tsx (5 hunks)
  • libs/service-portal/health/src/screens/Dentists/Dentists.tsx (1 hunks)
  • libs/service-portal/health/src/screens/HealthCenterRegistration/HealthCenterRegistration.tsx (4 hunks)
💤 Files with no reviewable changes (3)
  • libs/service-portal/health/src/components/RegisterModal/RegisterModal.css.ts
  • libs/service-portal/health/src/components/RegisterModal/RegisterModal.tsx
  • libs/service-portal/health/src/components/RegisterModal/index.ts
🧰 Additional context used
📓 Path-based instructions (6)
libs/service-portal/core/src/components/Modal/Modal.css.ts (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

  • Reusability of components and hooks across different NextJS apps.
  • TypeScript usage for defining props and exporting types.
  • Effective tree-shaking and bundling practices."
libs/service-portal/core/src/components/Modal/Modal.tsx (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

  • Reusability of components and hooks across different NextJS apps.
  • TypeScript usage for defining props and exporting types.
  • Effective tree-shaking and bundling practices."
libs/service-portal/health/src/lib/messages.ts (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

  • Reusability of components and hooks across different NextJS apps.
  • TypeScript usage for defining props and exporting types.
  • Effective tree-shaking and bundling practices."
libs/service-portal/health/src/screens/DentistRegistration/DentistRegistration.tsx (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

  • Reusability of components and hooks across different NextJS apps.
  • TypeScript usage for defining props and exporting types.
  • Effective tree-shaking and bundling practices."
libs/service-portal/health/src/screens/Dentists/Dentists.tsx (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

  • Reusability of components and hooks across different NextJS apps.
  • TypeScript usage for defining props and exporting types.
  • Effective tree-shaking and bundling practices."
libs/service-portal/health/src/screens/HealthCenterRegistration/HealthCenterRegistration.tsx (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

  • Reusability of components and hooks across different NextJS apps.
  • TypeScript usage for defining props and exporting types.
  • Effective tree-shaking and bundling practices."
🪛 Biome
libs/service-portal/core/src/components/Modal/Modal.tsx

[error] 59-59: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

libs/service-portal/health/src/screens/DentistRegistration/DentistRegistration.tsx

[error] 218-218: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

libs/service-portal/health/src/screens/HealthCenterRegistration/HealthCenterRegistration.tsx

[error] 138-138: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (5)
libs/service-portal/core/src/components/Modal/Modal.css.ts (1)

Line range hint 1-39: Changes align well with PR objectives

The addition of the image style to the Modal component aligns well with the PR objective of generalizing the modal and providing more customization options. This change contributes to standardizing the appearance of modals across the service portal, potentially improving the consistency of the user experience.

The implementation adheres to the coding guidelines for libs/**/* files:

  1. It enhances the reusability of the Modal component across different NextJS apps.
  2. It uses TypeScript for defining styles.
  3. The responsive design approach supports effective tree-shaking and bundling practices.
libs/service-portal/health/src/lib/messages.ts (1)

Line range hint 1-699: LGTM! File structure adheres to coding guidelines.

The file structure and implementation adhere to the coding guidelines for libs/**/* files:

  • Uses TypeScript for defining the messages object.
  • Messages are reusable across different NextJS apps.
  • The file structure supports effective tree-shaking and bundling.
libs/service-portal/health/src/screens/DentistRegistration/DentistRegistration.tsx (1)

79-79: Verify the hardcoded canRegister value

Setting canRegister to true removes any conditional logic that may have existed previously. Please verify if this change is intentional and that it doesn't bypass any necessary checks for dentist registration eligibility.

libs/service-portal/health/src/screens/HealthCenterRegistration/HealthCenterRegistration.tsx (2)

358-376: Ensure the Select component handles empty doctor list gracefully

When healthCenterDoctors is empty or undefined, the Select component will not render. Confirm that this behavior is intended and consider providing user feedback if no doctors are available for the selected health center.

If needed, you can display a message indicating that no doctors are available:

      {healthCenterDoctors?.length ? (
        <Box marginBottom={4}>
          <Select
            // existing props
          />
        </Box>
+     ) : (
+       <Text variant="small">
+         {formatMessage(messages.noDoctorsAvailable)}
+       </Text>
      ) : null}

134-146: ⚠️ Potential issue

Validate selection of a doctor before transferring health center

In the handleHealthCenterTransfer function, selectedHealthCenterDoctor may be undefined if the user hasn't selected a doctor. Ensure that a doctor is selected before proceeding with the transfer to prevent potential errors.

Consider adding validation to check if selectedHealthCenterDoctor is defined:

      const handleHealthCenterTransfer = async () => {
        setLoadingTransfer(true)
-       if (selectedHealthCenter?.id) {
+       if (selectedHealthCenter?.id && selectedHealthCenterDoctor) {

Or provide a default value or prompt the user if no doctor is selected.

🧰 Tools
🪛 Biome

[error] 138-138: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 8

🧹 Outside diff range and nitpick comments (4)
libs/service-portal/core/src/components/Modal/Modal.css.ts (1)

31-39: LGTM! Consider adding a comment for clarity.

The new image style is well-implemented, utilizing responsive styling and theme values for consistency. It adheres to the coding guidelines by being reusable, implicitly using TypeScript through vanilla-extract, and supporting effective tree-shaking.

Consider adding a brief comment explaining the purpose of this style, e.g.:

// Style for optional image in modal, visible only on large screens
export const image = style({
  // ... (existing code)
})

This would enhance code readability and maintainability.

libs/service-portal/health/src/lib/messages.ts (2)

699-700: LGTM: New success messages for health center registration transfer.

The added messages healthCenterRegistrationTransferSuccessTitle and healthCenterRegistrationTransferSuccessInfo are well-structured and provide clear information about the successful registration transfer. This addition aligns with the PR objective of enhancing the modal component for health center registration.

Consider adjusting the naming convention to be consistent with the dentist registration messages:

-  healthCenterRegistrationTransferSuccessTitle: {
+  healthCenterRegistrationTransferSuccessTitle: {
   id: 'sp.health:health-center-registration-transfer-success-title',
   defaultMessage: 'Ný heilsugæsla skráð',
 },
-  healthCenterRegistrationTransferSuccessInfo: {
+  healthCenterRegistrationTransferSuccessInfo: {
   id: 'sp.health:health-center-registration-transfer-success-info',
   defaultMessage: 'Þú hefur verið skráður á',
 },

This change would make the naming more consistent throughout the file.


699-699: Consider removing the unnecessary empty line.

The added empty line at 699 doesn't seem to separate any logical sections of the code. To maintain consistency with the rest of the file, consider removing this line.

-
 healthCenterRegistrationTransferSuccessTitle: {
   id: 'sp.health:health-center-registration-transfer-success-title',
   defaultMessage: 'Ný heilsugæsla skráð',
 },
libs/service-portal/health/src/screens/DentistRegistration/DentistRegistration.tsx (1)

199-199: Update modal icon to match context

The iconSrc is set to "./assets/images/coffee.svg", which may not be relevant to dentist registration. Consider using an icon that reflects dental services for better user experience.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 81e07f5 and 24f07b5.

📒 Files selected for processing (9)
  • libs/service-portal/core/src/components/Modal/Modal.css.ts (1 hunks)
  • libs/service-portal/core/src/components/Modal/Modal.tsx (5 hunks)
  • libs/service-portal/health/src/components/RegisterModal/RegisterModal.css.ts (0 hunks)
  • libs/service-portal/health/src/components/RegisterModal/RegisterModal.tsx (0 hunks)
  • libs/service-portal/health/src/components/RegisterModal/index.ts (0 hunks)
  • libs/service-portal/health/src/lib/messages.ts (2 hunks)
  • libs/service-portal/health/src/screens/DentistRegistration/DentistRegistration.tsx (5 hunks)
  • libs/service-portal/health/src/screens/Dentists/Dentists.tsx (1 hunks)
  • libs/service-portal/health/src/screens/HealthCenterRegistration/HealthCenterRegistration.tsx (4 hunks)
💤 Files with no reviewable changes (3)
  • libs/service-portal/health/src/components/RegisterModal/RegisterModal.css.ts
  • libs/service-portal/health/src/components/RegisterModal/RegisterModal.tsx
  • libs/service-portal/health/src/components/RegisterModal/index.ts
🧰 Additional context used
📓 Path-based instructions (6)
libs/service-portal/core/src/components/Modal/Modal.css.ts (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

  • Reusability of components and hooks across different NextJS apps.
  • TypeScript usage for defining props and exporting types.
  • Effective tree-shaking and bundling practices."
libs/service-portal/core/src/components/Modal/Modal.tsx (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

  • Reusability of components and hooks across different NextJS apps.
  • TypeScript usage for defining props and exporting types.
  • Effective tree-shaking and bundling practices."
libs/service-portal/health/src/lib/messages.ts (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

  • Reusability of components and hooks across different NextJS apps.
  • TypeScript usage for defining props and exporting types.
  • Effective tree-shaking and bundling practices."
libs/service-portal/health/src/screens/DentistRegistration/DentistRegistration.tsx (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

  • Reusability of components and hooks across different NextJS apps.
  • TypeScript usage for defining props and exporting types.
  • Effective tree-shaking and bundling practices."
libs/service-portal/health/src/screens/Dentists/Dentists.tsx (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

  • Reusability of components and hooks across different NextJS apps.
  • TypeScript usage for defining props and exporting types.
  • Effective tree-shaking and bundling practices."
libs/service-portal/health/src/screens/HealthCenterRegistration/HealthCenterRegistration.tsx (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

  • Reusability of components and hooks across different NextJS apps.
  • TypeScript usage for defining props and exporting types.
  • Effective tree-shaking and bundling practices."
🪛 Biome
libs/service-portal/core/src/components/Modal/Modal.tsx

[error] 59-59: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

libs/service-portal/health/src/screens/DentistRegistration/DentistRegistration.tsx

[error] 218-218: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

libs/service-portal/health/src/screens/HealthCenterRegistration/HealthCenterRegistration.tsx

[error] 138-138: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (8)
libs/service-portal/health/src/screens/Dentists/Dentists.tsx (1)

53-53: ⚠️ Potential issue

Reconsider the change to canRegister

The modification of canRegister to a constant true value raises some concerns:

  1. It removes the ability to disable registration based on the dentist's status, which may not align with the intended business logic.
  2. This change seems to contradict the PR objective of "providing more customization options" as it removes a dynamic behavior.
  3. A previous review comment suggested reverting this change, indicating potential issues.

Could you please clarify the reasoning behind this change? If there are specific use cases or requirements that necessitate this modification, it would be helpful to document them in the code or PR description.

To verify the impact of this change, we can run the following script:

This will help us understand if this change affects other parts of the codebase and if there are any conditional renderings that depend on canRegister.

✅ Verification successful

canRegister Change Verified Successfully

The modification of canRegister to a constant true in Dentists.tsx has been reviewed thoroughly. No conditional renderings or dependencies on canRegister were found that would be affected by this change outside of the targeted file.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for other occurrences of canRegister in the codebase

# Search for other usages of canRegister
echo "Searching for other usages of canRegister:"
rg --type typescript "canRegister" -g '!**/Dentists.tsx'

# Search for potential conditional rendering based on canRegister
echo "Searching for conditional rendering based on canRegister:"
ast-grep --lang typescript --pattern 'if ($canRegister) { $$$ }'

Length of output: 378


Script:

#!/bin/bash
# Description: Check for other occurrences of canRegister in the codebase

# Search for other usages of canRegister
echo "Searching for other usages of canRegister:"
rg --type ts "canRegister" -g '!**/Dentists.tsx'

echo "Searching for other usages of canRegister in .tsx files:"
rg --type tsx "canRegister" -g '!**/Dentists.tsx'

# Search for potential conditional rendering based on canRegister
echo "Searching for conditional rendering based on canRegister:"
ast-grep --lang typescript --pattern 'if ($canRegister) { $$$ }'

Length of output: 5387


Script:

#!/bin/bash
# Description: Check for other occurrences of canRegister in the codebase

# Search for other usages of canRegister in .ts files
echo "Searching for other usages of canRegister in .ts files:"
rg "canRegister" --glob "*.ts" -g '!**/Dentists.tsx'

# Search for other usages of canRegister in .tsx files
echo "Searching for other usages of canRegister in .tsx files:"
rg "canRegister" --glob "*.tsx" -g '!**/Dentists.tsx'

# Search for potential conditional rendering based on canRegister
echo "Searching for conditional rendering based on canRegister:"
ast-grep --lang typescript --pattern 'if ($canRegister) { $$$ }'

Length of output: 5393

libs/service-portal/health/src/lib/messages.ts (2)

284-288: LGTM: New error message for dentist registration transfer.

The added message dentistRegistrationTransferErrorInfo is well-structured and provides clear information about the error scenario. This addition aligns with the PR objective of enhancing the modal component for dentist registration.


Line range hint 1-1068: Summary: Changes align well with PR objectives.

The additions to this file, including new messages for dentist and health center registration transfers, align well with the PR objective of enhancing the modal component for these use cases. The new messages provide clear and informative content for both success and error scenarios.

A few minor suggestions were made to improve consistency in naming conventions and formatting. Overall, these changes contribute positively to the standardization of modals across the service portal.

libs/service-portal/core/src/components/Modal/Modal.tsx (1)

21-31: Props are well-defined and enhance component flexibility

The added props in lines 21-31 are correctly typed and will enhance the reusability and customization of the Modal component across different NextJS apps.

libs/service-portal/health/src/screens/DentistRegistration/DentistRegistration.tsx (3)

16-16: Import statements are appropriate

The imports of Modal and m from @island.is/service-portal/core are correct and align with their usage within the component.


42-42: State initialization for 'modalVisible' is correct

The addition of modalVisible state using useState<boolean>(false) effectively manages the visibility of the modal.


145-145: Error message reference updated correctly

Using messages.dentistRegistrationTransferErrorInfo ensures the error message is accurate and consistent with other localized messages.

libs/service-portal/health/src/screens/HealthCenterRegistration/HealthCenterRegistration.tsx (1)

307-310: Review the logic of the toggleClose prop

The toggleClose prop is set to !selectedHealthCenter, which may not match the intended behavior. Verify whether toggleClose should depend on selectedHealthCenter. This prop typically controls whether the modal can be closed by user actions like clicking outside or pressing 'Esc'. Ensure the modal's close behavior aligns with user expectations.

To confirm the behavior, please check the documentation of the Modal component and adjust the logic accordingly.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
libs/service-portal/health/src/screens/DentistRegistration/DentistRegistration.tsx (2)

198-260: Modal implementation looks good, with minor suggestions

The Modal component implementation aligns well with the PR objectives of generalizing and standardizing modals across the service portal. It's great to see the use of TypeScript for prop definitions, enhancing type safety. The component is reusable and highly configurable, which is excellent.

However, there are a couple of points to consider:

  1. The toggleClose prop is set to !modalVisible. This might not work as expected since it's not clear how the Modal component handles this prop internally. Consider using an isVisible prop instead if available, or clarify the usage of toggleClose.

  2. The condition for registering a dentist can be simplified using optional chaining.

Consider the following improvements:

  1. If the Modal component supports an isVisible prop:
<Modal
  id={'dentistRegisterModal'}
  initialVisibility={false}
- toggleClose={!modalVisible}
+ isVisible={modalVisible}
  // ... other props
>
  1. Simplify the dentist registration condition:
- if (selectedDentist && selectedDentist.id) {
+ if (selectedDentist?.id) {
  registerDentist({
    variables: {
      input: {
        id: `${selectedDentist.id}`,
      },
    },
  })
}

These changes will improve code clarity and follow modern TypeScript practices.

🧰 Tools
🪛 Biome

[error] 220-220: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


Line range hint 1-300: Overall implementation looks great, with a suggestion for enhancement

The changes to the DentistRegistration component successfully implement the PR objectives of generalizing the modal and standardizing its appearance across the service portal. The replacement of RegisterModal with the more generic Modal component enhances reusability and consistency.

Key improvements:

  1. Standardized modal implementation
  2. Enhanced state management for modal visibility
  3. Improved TypeScript usage and React best practices

To further enhance the component, consider the following:

  1. Error Handling: Implement specific error handling for modal operations, especially for the dentist registration process.
  2. Loading States: Add loading indicators within the modal during the registration process to improve user experience.
  3. Accessibility: Ensure the modal implementation follows accessibility best practices, such as proper focus management and keyboard navigation.

Example for adding a loading state:

const [isRegistering, setIsRegistering] = useState(false);

// In the registration function
const handleRegister = async () => {
  setIsRegistering(true);
  try {
    await registerDentist({
      variables: {
        input: {
          id: `${selectedDentist.id}`,
        },
      },
    });
  } catch (error) {
    // Handle error
  } finally {
    setIsRegistering(false);
  }
};

// In the Modal component
<Button
  // ... other props
  disabled={isRegistering}
>
  {isRegistering ? 'Registering...' : formatMessage(messages.healthRegisterModalAccept)}
</Button>

These enhancements will further improve the robustness and user experience of the component.

🧰 Tools
🪛 Biome

[error] 220-220: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 24f07b5 and 84e5436.

📒 Files selected for processing (1)
  • libs/service-portal/health/src/screens/DentistRegistration/DentistRegistration.tsx (4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/service-portal/health/src/screens/DentistRegistration/DentistRegistration.tsx (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

  • Reusability of components and hooks across different NextJS apps.
  • TypeScript usage for defining props and exporting types.
  • Effective tree-shaking and bundling practices."
🪛 Biome
libs/service-portal/health/src/screens/DentistRegistration/DentistRegistration.tsx

[error] 220-220: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (3)
libs/service-portal/health/src/screens/DentistRegistration/DentistRegistration.tsx (3)

16-16: LGTM: Modal import added correctly

The import of the Modal component from '@island.is/service-portal/core' is correctly placed and aligns with the coding guidelines for reusability across NextJS apps.


42-42: LGTM: Modal visibility state added

The addition of the modalVisible state using useState is well-implemented. It's correctly typed as a boolean and follows TypeScript usage guidelines. The naming is clear and descriptive, enhancing code readability.


147-147: LGTM: Error message updated for dentist context

The error message has been appropriately updated from 'healthCenterRegistrationTransferErrorInfo' to 'dentistRegistrationTransferErrorInfo'. This change correctly aligns the error message with the dentist registration context, improving user experience.

@thorkellmani thorkellmani marked this pull request as draft January 2, 2025 10:07
@thorkellmani thorkellmani deleted the feat/generalize-modal branch January 6, 2025 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy-feature Deploys features to dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants