Skip to content

[CCUBE-2221][ZZ] feat: migrate ESignature to Modal v2 - #1428

Open
ziggyzet wants to merge 12 commits into
masterfrom
ZZ/CCUBE-2221
Open

[CCUBE-2221][ZZ] feat: migrate ESignature to Modal v2#1428
ziggyzet wants to merge 12 commits into
masterfrom
ZZ/CCUBE-2221

Conversation

@ziggyzet

@ziggyzet ziggyzet commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Type of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing apis or functionality to change)
  • Documentation (change to documentation, comments or API descriptions)
  • Tests (improvements to unit tests or E2E tests)
  • Other (technical improvements, refactoring, or changes that don't fall into the above categories)

Description of changes

  • Link to ticket
  • Migrating Esignature to use modal v2. There are some screenshot updates after these changes.

Checklist

  • Changes follow the project guidelines in CONTRIBUTING.md and CONVENTIONS.md
  • Looks good on mobile and tablet
  • Updated documentation
  • Added/updated unit tests
  • Added/updated E2E tests

@ziggyzet
ziggyzet requested a review from qroll August 18, 2026 09:34
@ziggyzet ziggyzet self-assigned this Aug 18, 2026
@qroll
qroll requested a review from ghazwan-gt August 19, 2026 03:23
ghazwan-gt
ghazwan-gt previously approved these changes Aug 19, 2026
View reviewed changes
Comment thread src/e-signature/e-signature.styles.ts Outdated
Comment thread src/e-signature/e-signature.styles.ts Outdated
Comment thread src/e-signature/e-signature.styles.ts Outdated
Comment thread src/e-signature/e-signature.styles.ts Outdated
ghazwan-gt
ghazwan-gt previously approved these changes Aug 19, 2026
View reviewed changes
qroll
qroll reviewed Aug 19, 2026
View reviewed changes
Comment thread tests/e-signature/e-signature.spec.tsx
Comment on lines +102 to +105
await act(async () => {
jest.advanceTimersByTime(500);
});

@qroll qroll Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

wondering why the assertion on signature modal was removed, and we need to manually handle the timers here?

@ziggyzet ziggyzet Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the assertion was to check whether or not it's visible, but we're mount/unmounting for modal v2
modal v2 has a transition animation with 300ms, so that's just to skip the animation

@qroll qroll Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why is it not needed for the other tests?

Comment thread src/e-signature/e-signature.styles.ts Outdated
Comment thread src/e-signature/e-signature.styles.ts Outdated
Comment thread src/e-signature/e-signature.styles.ts Outdated
Comment thread src/e-signature/e-signature.styles.ts Outdated
Comment thread src/e-signature/e-signature.styles.ts Outdated
@qroll qroll added type: chore For technical improvements or refactoring. a11y Accessibility type: enhancement New feature or request and removed type: chore For technical improvements or refactoring. labels Aug 24, 2026
@ziggyzet
ziggyzet force-pushed the ZZ/CCUBE-2221 branch 2 times, most recently from 31d0194 to dae2cc2 Compare August 27, 2026 06:46
@ziggyzet
ziggyzet changed the base branch from master to ZZ/CCUBE-2221-fullscreen August 27, 2026 06:49
qroll
qroll requested changes Aug 31, 2026
View reviewed changes
Comment thread src/e-signature/e-signature.styles.ts Outdated
max-height: none;
margin: 0 auto;
padding: ${Spacing["spacing-16"]};
box-shadow: ${Shadow["lg-strong"]};

@qroll qroll Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what does the box shadow do?

@ziggyzet ziggyzet Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Modal.Box had this styling before but ModalV2.Card has xs-strong. Just making sure the styling is similar as the old one as much as possbile

@qroll qroll Sep 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

checking Figma, it is indeed meant to be shadow lg-strong. will create a separate ticket to align ModalV2

Comment thread src/e-signature/e-signature.styles.ts Outdated
Comment thread src/e-signature/e-signature.styles.ts Outdated
Comment thread src/e-signature/e-signature.styles.ts
width: 100%;
margin: auto;
padding: ${Spacing["layout-xxl"]} ${Spacing["layout-sm"]};
export const modalHeader = css`

@qroll qroll Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ideally with the new ModalV2.Header we no longer need to specify so many custom styles. what are the changes here and why are they needed?

@ziggyzet ziggyzet Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this was because the header fonts and spacing somehow after changing to using ModalV2 is way different to what we were using when it's still Modal

Comment thread src/e-signature/e-signature.styles.ts Outdated
Comment thread src/e-signature/e-signature.styles.ts
Base automatically changed from ZZ/CCUBE-2221-fullscreen to master September 1, 2026 04:18
qroll
qroll reviewed Sep 22, 2026
View reviewed changes
Comment thread src/e-signature/e-signature.styles.ts
Comment on lines +96 to +97
${MediaQuery.MaxWidth.sm} {
&[data-mobile-landscape] {

@qroll qroll Sep 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

are the selectors supposed to be nested? this means the screen width is sm AND the screen height is sm

in the previous selectors, they were comma separated which should have a OR logic

qroll
qroll reviewed Sep 22, 2026
View reviewed changes
Comment thread src/modal-v2/types.ts
* `closeButtonPosition`. Omit for a close-button-only header.
*/
title?: string | undefined;
title?: React.ReactNode | undefined;

@qroll qroll Sep 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Figma shows a standard heading xs now so the customisation is not needed

qroll
qroll reviewed Sep 22, 2026
View reviewed changes

@qroll qroll left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

still somewhat confused as to why some new styles are needed

testing against the latest master, I'm able to strip down most of the styles https://github.com/LifeSG/react-design-system/tree/RL/CCUBE-2221, with the main discrepancy being the header spacing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@qroll qroll qroll requested changes
@ghazwan-gt ghazwan-gt ghazwan-gt left review comments

Requested changes must be addressed to merge this pull request.

Assignees

@ziggyzet ziggyzet

Labels

a11y Accessibility type: enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ziggyzet @qroll @ghazwan-gt