Skip to content

chore(components): review and update prop validation across component library #4891

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

Draft
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

myrta2302
Copy link
Contributor

@myrta2302 myrta2302 commented Mar 6, 2025

PR Changes

  1. Replaced thrown errors with console errors
  2. Updated checkType function to handle NaN in case of type number
  3. Removed boolean checks
  4. Updated logic to:
  • a) if required: if checkNonEmpty false then checkType
  • b) if not required: keep checkEmptyOr.

@myrta2302 myrta2302 linked an issue Mar 6, 2025 that may be closed by this pull request
1 task
Copy link

changeset-bot bot commented Mar 6, 2025

🦋 Changeset detected

Latest commit: 3f61906

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 12 packages
Name Type
@swisspost/design-system-documentation Patch
@swisspost/design-system-components Patch
@swisspost/design-system-components-angular-workspace Patch
@swisspost/design-system-components-react Patch
@swisspost/design-system-components-angular Patch
@swisspost/design-system-nextjs-integration Patch
@swisspost/design-system-styles Patch
@swisspost/design-system-tokens Patch
@swisspost/design-system-icons Patch
@swisspost/design-system-styles-primeng Patch
@swisspost/internet-header Patch
@swisspost/design-system-styles-primeng-workspace Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@myrta2302 myrta2302 changed the title 4651 review prop validation across component library chore(components): review and update prop validation across component library Mar 6, 2025
@swisspost-bot
Copy link
Contributor

swisspost-bot commented Mar 6, 2025

Related Previews

@myrta2302 myrta2302 marked this pull request as ready for review March 6, 2025 14:06
@myrta2302 myrta2302 requested review from a team as code owners March 6, 2025 14:06
@myrta2302 myrta2302 requested review from gfellerph and removed request for gfellerph March 6, 2025 14:06
Copy link

@myrta2302 myrta2302 requested a review from alizedebray April 14, 2025 09:40
Copy link
Contributor

@alizedebray alizedebray left a comment

Choose a reason for hiding this comment

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

Is it possible to split the PR into smaller chunks?
Reviewing it is a little complex at the moment and I am not sure all changes are wanted.

item.setAttribute('heading-level', String(newValue));
});
validateHeadingLevel() {
if (!checkNonEmpty(this, 'headingLevel')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a convention for functions that return a boolean: https://dev.to/michi/tips-on-naming-boolean-variables-cleaner-code-35ig Can you try and apply it here?

Otherwise would you be able to know if the property is required or not by inspecting the this? If so the check empty/non empty could be done automatically in the checkers and not manually in the components.

this,
'headingLevel',
HEADING_LEVELS,
'The `heading-level` property of the `post-accordion` must be a number between 1 and 6.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this custom message necessary? The goal of this PR would be to remove them.

Copy link
Contributor

Choose a reason for hiding this comment

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

This file should probably not be deleted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is that change really part of your PR?

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