feat(education-notice): added component

[agliga]

Description

Added education notice new component

Context

For now, I used notice-base but we need to refactor it a bit next major.

References

#2120

Screenshots

[changeset-bot]

🦋 Changeset detected

Latest commit: 9a2007e

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

This PR includes changesets to release 1 package

Name	Type
@ebay/ebayui-core	Minor

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

[ArtBlue]

For reference, this is the Skin markup that includes all the optional sections:

<section class="education-notice" role="region" aria-label="Education" aria-roledescription="Notice">
    <div class="education-notice__header">
        <svg class="icon icon--the-ebay-vault-24" height="24" width="24" aria-label="Education">
            <use href="#icon-the-ebay-vault-24"></use>
        </svg>
        <h3 class="education-notice__title">The Vault</h3>
        <button aria-label="Close notice" class="fake-link education-notice__dismiss">
            <svg aria-hidden="true" class="icon icon--close-16" height="16" width="16">
                <use href="#icon-close-16"></use>
            </svg>
        </button>
    </div>
    <div class="education-notice__main">
        <p>You can now use the Vault to store items like this securely.</p>
    </div>
    <div class="education-notice__footer">
        <button class="fake-link">Learn more button</button>
    </div>
</section>

The button.fake-link can also be an anchor.

[agliga]

Not sure what you are trying to say here, users can add whatever they want here.

[ArtBlue]

The footer should only have a CTA; users should not be able to do whatever they want. I simply included a reference to the markup to demonstrate how it should be structured. education-notice is a lot more explicit how the sections should work and what they should have.

[agliga]

For now, will leave it as is.
There could be multiple ways a person would add a CTA, like they might want to add their own custom component for it, or use fake-link etc. This also requires us to handle all those events on the CTA like focus, click, link etc.
We have some places where we have it more explicit if its simple events, but for more complex interactions we allow users to pass in their component.

[ArtBlue]

If we do that, how can we ensure the styling from DS remains intact? What if they use a custom component that looks completely different and unsupported by DS and Skin? We're ok with the way things would look? Can't we just have a link with an href or a button that emits the click and the dev does whatever they want with the click?

[agliga]

We can, but we also need to allow them to change it to a link or button. If they want it to be a link, it should have href, otherwise we need to emit a click event.
This is what the code looks like in both cases:
Current

<ebay-education-notice>
   <@footer><ebay-fake-link on-click("trigger")>Link</></@footer>
</>

Proposed

<ebay-education-notice on-cta-button-click('trigger')>
   <@cta as="button" class="fake-link">Link</@cta>
</> 

I don't like how they have to add fake-link class if they set it to button personally. I think its better if they pass in a component.

[ArtBlue]

I see. In that case, can we add the fact that the component requires either an <a> or <ebay-fake-link> to look as it was intended. We're currently not enforcing those elements and nowhere that I can are we proving guidance for what it needs.

[ArtBlue]

I don't think this guidance applies to education-notice. It's different than section-notice. Maybe something like this:

Suggested change

	The `<ebay-education-notice>` is a tag used to create a custom-designed notice element. The notice can be single or multi-line but each line should be wrapped inside a `<p>` tag.
	The `<ebay-education-notice>` is a tag used to create an education notice. The notice requires a title and also a description which will automatically be wrapped inside a `<p>` tag.
	A notice can have a prominent variation.
	An ebay ui icon can be used as well. If one's not provided a default icon will be used. The icon can also have a prominent variation.
	The `education-notice` can be placed permanently on the page or dismissable.
	A notice can also optionally have a CTA action with an `<a>` as well as an `<ebay-fake-link>` button.

[agliga]

I think part of this is what we have in storybook already. We have prominent in stroybook already, we don't need it in two places.
We were getting to really longwinded readme's and we wanted to make them very brief. This info is usually found in storybook.

[ArtBlue]

I don't get it. This is where the users land when they "enter" into the component docs. What's wrong with having more complete info? I guess you can ignore my suggestion if you really want to, but we should avoid having inaccurate information.

According to the DS specs, this is simply not accurate and misleading: The notice can be single or multi-line

[ArtBlue]

This is still inaccurate: "The notice can be single or multi-line"

[agliga]

The `<ebay-education-notice>` is a tag used to create an education notice element. The notice requires a `@title` element and should be wrapped inside a `<p>` tag.

This is what I have currently in code. https://github.com/eBay/ebayui-core/pull/2125/files#diff-279dbdd569cd0d80d5c06efe3bd5f3e71f7376df7921f3ebd2453ad370182addR10

Do we want to add the notice can be single or multiline?

[agliga]

I think part of this as I mentioned in the description is done to get education notice out.
When we refactor our notices next major version (which we should have last major just did not have time), we will also refactor notice-base. But for now, keeping notice base like this because it is easier this way.

[ianmcburnie]

I think part of this as I mentioned in the description is done to get education notice out.

Is there a crazy rush/deliverable date? Typically we prefer not to rush things, especially if there are outstanding PR comments/concerns. Let's take the time to address those properly. We can always release this mid-SWU cycle for those that want to pick it up before the next SWU if we need.

I also understand that we might have to pick up some tech debt that will be cleaned up in next major.

[agliga]

I think part of this as I mentioned in the description is done to get education notice out.

Is there a crazy rush/deliverable date? Typically we prefer not to rush things, especially if there are outstanding PR comments/concerns. Let's take the time to address those properly. We can always release this mid-SWU cycle for those that want to pick it up before the next SWU if we need.

I also understand that we might have to pick up some tech debt that will be cleaned up in next major.

There is no rush, that was not what I meant.
I said that this is done this way to get education notice out so we dont have to spend a long time refactoring code and doing tech debt, which as I mentioned, we should, but just not now.
We can redo notice base completely but it seemed A) outside the scope of this and B) will be more tech debt until we fix the skin markup as well.
This was brought up before and Im just calling it out so we don't go on a tangent about that. That's all.

[ianmcburnie]

There is no rush, that was not what I meant.

Gotcha! Thanks for clarifying, Andrew.

[agliga]

Added icon button for close
There was a bug with tests which I fixed in 13.3.0. So reran tests.

[ArtBlue]

Looks good, just something about the icon button...

[agliga]

Looks good, just something about the icon button...

I updated icon-btn to use small for this PR. I saw I missed that.
From what I saw in your PR we are using icon button there I thought. It seems to look fine.