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(components): add alert component #1085

Merged
merged 34 commits into from
Sep 29, 2023
Merged

feat(components): add alert component #1085

merged 34 commits into from
Sep 29, 2023

Conversation

alizedebray
Copy link
Contributor

@oliverschuerch @gfellerph
I opened this PR as a draft so that we can discuss the following points:

  • Main question: Should we merge the new post-alert story with the existing alert story? If so, how?
    With the development of the web component library we will often face this question in the future, thus now is a good time to choose a documentation architecture that takes this duality into account.
  • Subsidiary question: The post-alert documentation currently uses Stencil generated controls. In your opinion, is it good as is or would it be better to customize it?

@alizedebray alizedebray linked an issue Feb 7, 2023 that may be closed by this pull request
@changeset-bot
Copy link

changeset-bot bot commented Feb 7, 2023

🦋 Changeset detected

Latest commit: 227a511

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

This PR includes changesets to release 8 packages
Name Type
@swisspost/design-system-styles Patch
@swisspost/design-system-documentation Minor
@swisspost/design-system-components Minor
@swisspost/design-system-components-angular Patch
@swisspost/design-system-demo Patch
@swisspost/internet-header Patch
@swisspost/design-system-intranet-header Patch
@swisspost/design-system-components-react 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

@swisspost-bot
Copy link
Contributor

swisspost-bot commented Feb 7, 2023

Preview environment ready: https://preview-1085--swisspost-design-system-next.netlify.app
Preview environment ready: https://preview-1085--swisspost-web-frontend.netlify.app

@sonarcloud
Copy link

sonarcloud bot commented Feb 20, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@alizedebray
Copy link
Contributor Author

@oliverschuerch @gfellerph I opened this PR as a draft so that we can discuss the following points:

* **Main question:** Should we merge the new post-alert story with the existing alert story? If so, how?
  With the development of the web component library we will often face this question in the future, thus now is a good time to choose a documentation architecture that takes this duality into account.

* **Subsidiary question:** The post-alert documentation currently uses Stencil generated controls. In your opinion, is it good as is or would it be better to customize it?

Main answer: we decided to go with a single docs page and use tabs to show examples for standard HTML and Web Components. This PR is therefore blocked by the tabs PR #1181
image

Subsidiary answer: we decided to use Stencil generated controls and customize them if necessary

@alizedebray
Copy link
Contributor Author

alizedebray commented Aug 8, 2023

TODO:

@imagoiq imagoiq removed their assignment Sep 26, 2023
@alizedebray alizedebray marked this pull request as ready for review September 26, 2023 16:30
@alizedebray alizedebray requested review from imagoiq and removed request for oliverschuerch and imagoiq September 26, 2023 16:33
Copy link
Contributor

@imagoiq imagoiq left a comment

Choose a reason for hiding this comment

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

Several bugs or questions:

  • Bug on WebC variant, Icon is displayed above text, but main icon does not change.
  • On primary type, when we change the icon, the color is black instead of white. This a bug already present in the current version, but I'm wondering if this will be maybe fixed with the PR's about mask icon?
  • Standard HTML alert is not dismissable (or at least there is no control anymore). It makes sense according to the recommendation that you wrote on the docs, but then it should be deprecated no?

.changeset/sweet-singers-trade.md Outdated Show resolved Hide resolved
@@ -35,9 +35,8 @@
}
],
"@stencil-community/strict-boolean-conditions": "off",
"@stencil-community/required-prefix": ["error", ["post"]],
"@stencil-community/required-prefix": ["error", ["post-"]],
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't is covered by another rule, the fact that web components should be named with a dash? At least the browser throw a DOMexception error with invalid name: https://webcomponents.guide/learn/components/naming-your-components/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but if you write postalert there is no error.

@@ -0,0 +1,3 @@
const alert = document.getElementById('alertId') as HTMLPostAlertElement;

alert.dismiss().then(() => {});
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
alert.dismiss().then(() => {});
alert.dismiss()

Nitpick: I'm not sure that it is necessary to show the then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is to highlight that the .dimiss() method returns a promise

packages/styles/src/components/alert.scss Show resolved Hide resolved
@alizedebray
Copy link
Contributor Author

Several bugs or questions:

  • Bug on WebC variant, Icon is displayed above text, but main icon does not change.

  • On primary type, when we change the icon, the color is black instead of white. This a bug already present in the current version, but I'm wondering if this will be maybe fixed with the PR's about mask icon?

  • Standard HTML alert is not dismissable (or at least there is no control anymore). It makes sense according to the recommendation that you wrote on the docs, but then it should be deprecated no?

  • I should have wrote a comment about your first point: the placement of the post-icon into the alerts is handle in the PR chore(styles, docs): re-apply mask icon changes #1945. We should merge it first.
  • The black icons are a known limitation because we do not provide pi-* classes for white icons, project have to handle them by there own. This is also why we decided to moved to the mask-image icons.
  • I would probably not write a deprecation message in the docs as they are the "next" documentation. Maybe we can create a ticket to deprecate them in the demo, what do you think?

Copy link
Contributor

@imagoiq imagoiq left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just fix the 3 code smells.

I would probably not write a deprecation message in the docs as they are the "next" documentation. Maybe we can create a ticket to deprecate them in the demo, what do you think?

Yes good idea.

@sonarcloud
Copy link

sonarcloud bot commented Sep 29, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@alizedebray
Copy link
Contributor Author

Ticket to deprecate dismissible HTML alerts: #2014

@alizedebray alizedebray merged commit d487840 into main Sep 29, 2023
10 checks passed
@alizedebray alizedebray deleted the 1006-component-alert branch September 29, 2023 08:20
gfellerph pushed a commit that referenced this pull request Oct 4, 2023
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @swisspost/design-system-components@1.5.0

### Minor Changes

- Added the `post-tooltip` component. (by
[@gfellerph](https://github.com/gfellerph) with
[#1879](#1879))

- Created the web component variant for the alert component. (by
[@alizedebray](https://github.com/alizedebray) with
[#1085](#1085))

### Patch Changes

-   Updated dependencies:
    -   @swisspost/design-system-styles@6.4.1

## @swisspost/internet-header@1.10.0

### Minor Changes

- Enabled nesting the header in a different scroll container than the
`<body>` element. The header stickyness and logo animation logic will
automatically attach to the nearest scrolling container instead of the
document when nested in a container that has `overflow: auto | scroll`
set. (by [@alizedebray](https://github.com/alizedebray) with
[#1855](#1855))

- Added ability to toggle programmatically an overlay associated with a
button using the `toggleOverlayById` method. (by
[@imagoiq](https://github.com/imagoiq) with
[#1838](#1838))

### Patch Changes

- Fixed an issue with custom configuration that was not applied when the
prop "language" was not set on the internet header. (by
[@alizedebray](https://github.com/alizedebray) with
[#1855](#1855))
-   Updated dependencies:
    -   @swisspost/design-system-styles@6.4.1

## @swisspost/design-system-intranet-header@5.0.1

### Patch Changes

- Removed bound between logo and language to allow any language to
display the logo. (by [@imagoiq](https://github.com/imagoiq) with
[#2009](#2009))
-   Updated dependencies:
    -   @swisspost/design-system-styles@6.4.1

## @swisspost/design-system-styles@6.4.1

### Patch Changes

- Removed bound between logo and language to allow any language to
display the logo. (by [@imagoiq](https://github.com/imagoiq) with
[#2009](#2009))

- Reduced the gap between the alert body and action buttons. (by
[@alizedebray](https://github.com/alizedebray) with
[#1085](#1085))

- Removed duplicated close button on toast when using ngx-toastr
programatically. (by [@imagoiq](https://github.com/imagoiq) with
[#2011](#2011))

- Updated and added the gaps between the title and the link-list in the
`.topic-teaser-content` class to comply with the figma design. (by
[@b1aserlu](https://github.com/b1aserlu) with
[#1927](#1927))

- Added missing pointer-events to allow a dismissible toast to be closed
interactively. (by [@imagoiq](https://github.com/imagoiq) with
[#2008](#2008))

- Updated icons to display as mask images, this way their color can be
set by adjusting the CSS `background-color` property. (by
[@alizedebray](https://github.com/alizedebray) with
[#1945](#1945))

## @swisspost/design-system-documentation@2.1.0

### Minor Changes

- Added the `post-tooltip` component. (by
[@gfellerph](https://github.com/gfellerph) with
[#1879](#1879))

- Integrated the google tag manager and the basic events (page_context,
page_change) to the documentation. (by
[@oliverschuerch](https://github.com/oliverschuerch) with
[#1951](#1951))

- Created the web component variant for the alert component. (by
[@alizedebray](https://github.com/alizedebray) with
[#1085](#1085))

### Patch Changes

- Used the post-icon component instead of `.pi` classes to display icons
in stories. (by [@alizedebray](https://github.com/alizedebray) with
[#1945](#1945))

- Added deprecation alerts for `.form-control-rg`, `.form-control-md`,
`.form-select-rg` and `.form-select-md` form-control variants. (by
[@b1aserlu](https://github.com/b1aserlu) with
[#1882](#1882))

-   Updated dependencies:
    -   @swisspost/design-system-components@1.5.0
    -   @swisspost/design-system-styles@6.4.1
    -   @swisspost/internet-header@1.10.0
    -   @swisspost/design-system-components-react@1.0.18

## @swisspost/design-system-components-angular@3.0.1

### Patch Changes

-   Updated dependencies:
    -   @swisspost/design-system-styles@6.4.1

## @swisspost/design-system-components-react@1.0.18

### Patch Changes

-   Updated dependencies:
    -   @swisspost/design-system-components@1.5.0

## @swisspost/design-system-demo@7.0.1

### Patch Changes

- Updated the installation intructions for the Intranet Header. (by
[@alizedebray](https://github.com/alizedebray) with
[#1942](#1942))

- Added deprecation alerts for `.form-control-rg`, `.form-control-md`,
`.form-select-rg` and `.form-select-md` form-control variants. (by
[@b1aserlu](https://github.com/b1aserlu) with
[#1882](#1882))

- Fixed ngBootstrap documentation links. (by
[@imagoiq](https://github.com/imagoiq) with
[#1987](#1987))

-   Updated dependencies:
    -   @swisspost/design-system-intranet-header@5.0.1
    -   @swisspost/design-system-styles@6.4.1
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.

Component: Alert
4 participants