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

[ENT-8491] Implement Base Link Learners modal #1184

Merged
merged 11 commits into from
Mar 20, 2024
Merged

Conversation

kiram15
Copy link
Contributor

@kiram15 kiram15 commented Mar 11, 2024

This is the first pass at the invite learner modal for groups. This work includes the validation of the emails (duplicates allowed at this point but warned against), misformatted should surface an error and block.

Jira ticket

Screen.Recording.2024-03-11.at.4.27.18.PM.mov

For all changes

  • Ensure adequate tests are in place (or reviewed existing tests cover changes)

Only if submitting a visual change

  • Ensure to attach screenshots
  • Ensure to have UX team confirm screenshots

Copy link

codecov bot commented Mar 11, 2024

Codecov Report

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

Project coverage is 85.44%. Comparing base (1ea9233) to head (15d9226).

Files Patch % Lines
...agement/invite-modal/InviteMembersModalWrapper.jsx 80.55% 7 Missing ⚠️
.../hooks/useSuccessfulInvitationToastContextValue.js 71.42% 4 Missing ⚠️
...ent/invite-modal/InviteModalSummaryLearnerList.jsx 81.25% 3 Missing ⚠️
src/data/services/LmsApiService.js 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1184      +/-   ##
==========================================
+ Coverage   85.38%   85.44%   +0.06%     
==========================================
  Files         498      505       +7     
  Lines       10864    11023     +159     
  Branches     2291     2318      +27     
==========================================
+ Hits         9276     9419     +143     
- Misses       1544     1560      +16     
  Partials       44       44              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

<Button variant="tertiary" onClick={handleCloseInviteModal}>Cancel</Button>
<StatefulButton
labels={{
default: 'Invite',
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Can we break out the button states into constant(s)?

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 think personally it would make it less readable, and I wanted to follow the pattern set from this

Copy link
Member

Choose a reason for hiding this comment

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

Also, worth nothing that if/when i18n is introduced into this MFE, any English strings stored in constants will need to be replaced with intl.formatMessage or FormattedMessage, etc.

<Button variant="tertiary" onClick={handleCloseInviteModal}>Cancel</Button>
<StatefulButton
labels={{
default: 'Invite',
Copy link
Member

Choose a reason for hiding this comment

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

Also, worth nothing that if/when i18n is introduced into this MFE, any English strings stored in constants will need to be replaced with intl.formatMessage or FormattedMessage, etc.

src/data/services/EnterpriseCatalogApiService.js Outdated Show resolved Hide resolved
src/utils.js Outdated Show resolved Hide resolved
@kiram15 kiram15 merged commit 37e3151 into master Mar 20, 2024
6 checks passed
@kiram15 kiram15 deleted the kiram15/ENT-8491 branch March 20, 2024 16:11
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