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(alert-banner): add alert banner component #4266

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

loredanaspataru
Copy link

@loredanaspataru loredanaspataru commented Apr 12, 2024

Description

Create alert-banner component

Related issue(s)

Motivation and context

How has this been tested?

Screenshots (if appropriate)

Screenshot 2024-04-19 at 14 27 30

Types 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 functionality to change)
  • Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Checklist

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • If my change required a change to the documentation, I have updated the documentation in this pull request.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

Best practices

This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against main.

Copy link

github-actions bot commented Apr 15, 2024

Lighthouse scores

Category Latest (report) Main (report) Branch (report)
Performance 0.97 0.99 0.99
Accessibility 1 1 1
Best Practices 1 1 1
SEO 1 0.92 0.92
PWA 1 1 1
What is this?

Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on main ("Main"). Higher scores are better, but note that the SEO scores on Netlify URLs are artifically constrained to 0.92.

Transfer Size

Category Latest Main Branch
Total 229.455 kB 210.239 kB 🏆 210.683 kB
Scripts 61.241 kB 48.019 kB 🏆 48.321 kB
Stylesheet 35.543 kB 30.434 kB 🏆 30.526 kB
Document 5.959 kB 5.189 kB 🏆 5.204 kB
Font 126.712 kB 126.597 kB 🏆 126.632 kB

Request Count

Category Latest Main Branch
Total 43 🏆 45 45
Scripts 35 🏆 37 37
Stylesheet 5 5 5
Document 1 1 1
Font 2 2 2

Copy link

github-actions bot commented Apr 15, 2024

Tachometer results

Chrome

action-bar permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 483 kB 49.31ms - 50.80ms - faster ✔
1% - 5%
0.55ms - 2.40ms
branch 472 kB 50.97ms - 52.08ms slower ❌
1% - 5%
0.55ms - 2.40ms
-

action-menu permalink

test-basic

Version Bytes Avg Time vs remote vs branch
npm latest 646 kB 134.41ms - 138.18ms - faster ✔
3% - 7%
4.94ms - 9.89ms
branch 634 kB 142.10ms - 145.32ms slower ❌
4% - 7%
4.94ms - 9.89ms
-

test-directive permalink

Version Bytes Avg Time vs remote vs branch
npm latest 603 kB 61.59ms - 63.24ms - faster ✔
4% - 7%
2.82ms - 4.95ms
branch 591 kB 65.63ms - 66.98ms slower ❌
4% - 8%
2.82ms - 4.95ms
-

test-lazy permalink

Version Bytes Avg Time vs remote vs branch
npm latest 602 kB 59.76ms - 61.00ms - faster ✔
6% - 10%
4.17ms - 6.31ms
branch 590 kB 64.75ms - 66.49ms slower ❌
7% - 11%
4.17ms - 6.31ms
-

test-open-close-directive permalink

Version Bytes Avg Time vs remote vs branch
npm latest 787 kB 1870.83ms - 1874.11ms - unsure 🔍
-0% - -0%
-7.66ms - -3.07ms
branch 776 kB 1876.23ms - 1879.44ms unsure 🔍
+0% - +0%
+3.07ms - +7.66ms
-

test-open-close permalink

Version Bytes Avg Time vs remote vs branch
npm latest 786 kB 1859.34ms - 1862.21ms - unsure 🔍
-0% - -0%
-5.47ms - -0.84ms
branch 774 kB 1862.11ms - 1865.75ms unsure 🔍
+0% - +0%
+0.84ms - +5.47ms
-
Firefox

action-bar permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 483 kB 108.55ms - 114.53ms - unsure 🔍
-6% - +1%
-7.02ms - +1.14ms
branch 472 kB 111.71ms - 117.25ms unsure 🔍
-1% - +6%
-1.14ms - +7.02ms
-

action-menu permalink

test-basic

Version Bytes Avg Time vs remote vs branch
npm latest 646 kB 267.32ms - 269.96ms - faster ✔
12% - 13%
35.68ms - 40.80ms
branch 634 kB 304.69ms - 309.07ms slower ❌
13% - 15%
35.68ms - 40.80ms
-

test-directive permalink

Version Bytes Avg Time vs remote vs branch
npm latest 603 kB 122.74ms - 124.38ms - faster ✔
3% - 5%
3.97ms - 6.35ms
branch 591 kB 127.86ms - 129.58ms slower ❌
3% - 5%
3.97ms - 6.35ms
-

test-lazy permalink

Version Bytes Avg Time vs remote vs branch
npm latest 602 kB 146.12ms - 151.40ms - slower ❌
6% - 11%
8.83ms - 15.45ms
branch 590 kB 134.62ms - 138.62ms faster ✔
6% - 10%
8.83ms - 15.45ms
-

test-open-close-directive permalink

Version Bytes Avg Time vs remote vs branch
npm latest 787 kB 1909.05ms - 1916.67ms - slower ❌
1% - 2%
22.69ms - 31.35ms
branch 776 kB 1883.79ms - 1887.89ms faster ✔
1% - 2%
22.69ms - 31.35ms
-

test-open-close permalink

Version Bytes Avg Time vs remote vs branch
npm latest 786 kB 1881.09ms - 1885.55ms - unsure 🔍
-0% - +0%
-3.90ms - +2.82ms
branch 774 kB 1881.34ms - 1886.38ms unsure 🔍
-0% - +0%
-2.82ms - +3.90ms
-

@loredanaspataru loredanaspataru marked this pull request as ready for review April 15, 2024 14:28
@Rajdeepc Rajdeepc requested a review from a team April 16, 2024 04:20
Copy link
Contributor

@Rajdeepc Rajdeepc left a comment

Choose a reason for hiding this comment

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

Nice start!! Few stories and tests are missing which would be a great addition to the element.

packages/alert-banner/src/AlertBanner.ts Show resolved Hide resolved
packages/alert-banner/src/AlertBanner.ts Show resolved Hide resolved
packages/alert-banner/stories/alert-banner.stories.ts Outdated Show resolved Hide resolved
packages/alert-banner/test/alert-banner.test.ts Outdated Show resolved Hide resolved
@Rajdeepc Rajdeepc requested a review from a team April 19, 2024 11:00
packages/alert-banner/stories/alert-banner.stories.ts Outdated Show resolved Hide resolved
packages/alert-banner/src/AlertBanner.ts Outdated Show resolved Hide resolved
packages/alert-banner/src/AlertBanner.ts Outdated Show resolved Hide resolved
packages/alert-banner/src/AlertBanner.ts Outdated Show resolved Hide resolved
packages/alert-banner/src/AlertBanner.ts Outdated Show resolved Hide resolved
packages/alert-banner/src/AlertBanner.ts Outdated Show resolved Hide resolved
@import url('./spectrum-alert-banner.css');

:host {
--spectrum-alert-banner-size: 100%;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like the sort of thing we should raise as an error in the Spectrum CSS implementation.

Copy link
Author

Choose a reason for hiding this comment

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

Got it, I removed from our styles and will raise this to spectrum-css

@adobe adobe deleted a comment from changeset-bot bot Apr 25, 2024
@Rajdeepc
Copy link
Contributor

Rajdeepc commented Apr 29, 2024

Please check the below issues by changing different device types.

Screenshot 2024-04-29 at 2 55 37 PM Screenshot 2024-04-29 at 2 55 43 PM Screenshot 2024-04-29 at 2 55 23 PM

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.

None yet

3 participants