Closed kiley-mitti closed 1 year ago
changeset-bot[bot]
commented
2 years ago Latest commit: 5e87f59cccfb91e7574ee83b08e3937177ff7b0d
The changes in this PR will be included in the next version bump.
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
netlify[bot]
commented
2 years ago | Name | Link |
|---|---|
| Latest commit | 5e87f59cccfb91e7574ee83b08e3937177ff7b0d |
| Latest deploy log | https://app.netlify.com/sites/astrouxds/deploys/6356fd809841c00008990943 |
netlify[bot]
commented
2 years ago | Name | Link |
|---|---|
| Latest commit | 5e87f59cccfb91e7574ee83b08e3937177ff7b0d |
| Latest deploy log | https://app.netlify.com/sites/astro-preview/deploys/6356fd806f0c9d00096ef534 |
| Deploy Preview | https://deploy-preview-854--astro-preview.netlify.app |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site settings.
netlify[bot]
commented
2 years ago | Name | Link |
|---|---|
| Latest commit | 5e87f59cccfb91e7574ee83b08e3937177ff7b0d |
| Latest deploy log | https://app.netlify.com/sites/blissful-hugle-92e0cf/deploys/6356fd80a01bcf000d5b8507 |
markacianfrani
commented
2 years ago I agree we should just remove the prop. Keeping this open for now bc I want to think about some ways to improve our process of deprecating and breaking changes.
markacianfrani
commented
1 year ago Created ASTRO-4935 to circle back on this after we decide if we're going to change our versioning strat. Closing for now
Brief Description
Removed the message prop from rux-notification. This is a major breaking change.
JIRA Link
ASTRO-4780
General Notes
The Problem When a named slot is added to the fallback prop 'message' for the default slot is no longer displayed.
For example:
<rux-notification open status="standby" message="Insert better example here"><rux-icon icon="apps" slot="prefix"></rux-icon></rux-notification>Should display the 'apps' icon to the left of a standby icon and the message 'Insert better example here' to the right. However, it doesn't display the message.
The Reason This is actually expected web component behavior, even though at a glance it doesn't seem like that. the prop "message" displays as a fallback for the default slot. When default slot is empty it should display. However, the default slot is not actually empty. There are hidden spaces and carriage returns surrounding
<rux-notification open status="standby" message="Insert better example here"><rux-icon icon="apps" slot="prefix"></rux-icon></rux-notification>The 'message' should then appear as expected.
Source
The Solution One possibility would be to write a method that seeks out and ignores these spaces and carriage returns.. but that could get complicated and messy quite quickly.
Instead, I've chosen to remove the message prop and change the instructions for this component. I believe that this is a better choice on a couple of levels.
1) I think that adding the message inside of theNotification Message here! is more intuitive in the long run. Using props as fallback for text can get confusing in corner cases. For example, what if a dev utilizes both the message prop and the default slot?
<rux-notification message="This is my notification">J/K It's here</rux-notification>What is the expected outcome? Should it render both? Just the default slot? Just the message?2) Our storybook on notification actually already calls out that things put into the default slot will override 'message' in this case it's just doing it for an unexpected reason.
3) Other comparable web-component libraries that use slots for this component handle the main content of an alert or notification component in a similar manner. shoelace, kor, or do not list a default slot: Carbon
Issues and Limitations
While this is a simple fix, it is going to be a big ugly breaking change when it goes into production, so if we decide to implement it later and just add some documentation for now, or go a different direction entirely, I can test out other solutions. :)
Types of changes
Checklist