[cxl-notification]

[anoblet]

https://app.clickup.com/t/3d9ttgy

[github-actions[bot]]

size-limit report 📦

Path	Size
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js, packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js, packages/cxl-ui/pkg/dist-web/cxl-ui.js	59.3 KB (+0.69% 🔺)

[saas786]

@anoblet

• test & squash commits in this PR
• Merge: https://github.com/conversionxl/aybolit/pull/219 into this PR
• also rebase it from master branch.

I will review & test it one last time.

Looks good for now.

[saas786]

I looked at netlify preview and noticed that x is not aligned correctly in close button. I checked new notification but also GF.

@pawelkmpt its not a part of cxl-notification itself, but rather focused on Gravity Forms styles...

You can see in screenshot below, that margin-top is applied, also it could use smaller font size as well.

[saas786]

Issue created for Gravity Forms styles in notification: https://github.com/conversionxl/aybolit/issues/225

[pawelkmpt]

@pawelkmpt its not a part of cxl-notification itself, but rather focused on Gravity Forms styles...

But this part is about notification. x inside the red box is not in the middle.

I looked at netlify preview and noticed that x is not aligned correctly in close button. I checked new notification but also GF.

You can see in screenshot below, that margin-top is applied, also it could use smaller font size as well.

I don't think top margin should have impact on centering x inside the button.

[saas786]

@pawelkmpt its not a part of cxl-notification itself, but rather focused on Gravity Forms styles...

But this part is about notification. x inside the red box is not in the middle.

I looked at netlify preview and noticed that x is not aligned correctly in close button. I checked new notification but also GF.

You can see in screenshot below, that margin-top is applied, also it could use smaller font size as well.

I don't think top margin should have impact on centering x inside the button.

Close icon is usually at top right, not middle aligned.

We can center align it, but it would only look nice for single line notifications, multiline it would not good aligned or good.

[pawelkmpt]

Close icon is usually at top right, not middle aligned.

We can center align it, but it would only look nice for single line notifications, multiline it would not good aligned or good.

You don't get what I mean by "x inside the red box is not in the middle".

BAD

GOOD

[saas786]

Close icon is usually at top right, not middle aligned. We can center align it, but it would only look nice for single line notifications, multiline it would not good aligned or good.

You don't get what I mean by "x inside the red box is not in the middle".

BAD

GOOD

Correct, I didn't understood it :), thanks for re-explaining.

But it still only applicable to GravityForms styles...

See un-paid order notification button, centered.

See Gravity Forms notification, and un-paid notifcation buttons, but have same styles applied, only calculated width is different, which is causing it to look mis-aligned.

---

So can delegate it to https://github.com/conversionxl/aybolit/issues/225

[pawelkmpt]

Correct, I didn't understood it :), thanks for re-explaining.

But it still only applicable to GravityForms styles...

See un-paid order notification button, centered.

Issue is present for notification as well on mobile screens. I should've mentioned that in the beginning. Tablet and desktop indeed look good.

So can delegate it to https://github.com/conversionxl/aybolit/issues/225

It has to be done here.

[anoblet]

I'm not sure why our styles differ from the Vaadin demo (https://vaadin.com/docs/latest/components/button). It seems to only happen on the gravity forms stories, and not the default one. Applying this rule to the gravity forms stories gives us a button with a centered icon:

vaadin-button {
  min-width: initial;
}

[anoblet]

I found the culprit. This h2 is stretching the button somehow: https://github.com/conversionxl/aybolit/blob/6de178f17450a6a2e868b9c8d994cea14943e4fb/packages/storybook/cxl-ui/cxl-notification/gravity-form.story.js#L17-L20

If you replace this with a simple div, the icon is centered.

[anoblet]

This should be fixed by wrapping the button in a div.

[lkraav]

I found the culprit. This h2 is stretching the button somehow

Why would the button be inside heading? Super weird structure.

[anoblet]

@lkraav

For the default story, I took the structure from the Vaadin demo (https://vaadin.com/docs/latest/components/notification/#error).

Vaadin doesn't recommend long content for notifications (https://vaadin.com/docs/latest/components/notification/#limit-content-length). The Gravity Form story content is pretty large, and I don't believe the demo structure supports it.

[saas786]

I found the culprit. This h2 is stretching the button somehow

Why would the button be inside heading? Super weird structure.

Its not inside <h2> but rather alongside it, as sibling.

[saas786]

This should be fixed by wrapping the button in a div.

@anoblet

Patched the <vaadin-button>, I added it to global styles, couldn't find proper selector to make it specific for <cxl-notification>, can you please look into making it so it applies this style when <cxl-notification><vaadin-button>.. its wrapped inside <cxl-notification>.

Inspired from:

[anoblet]

@saas786 I had it fixed in the commit I pushed.

The h2 stretches the content so that it spans multiple lines. This affects the dimensions of the button. The reason the default story doesn't have this issue is because it only spans one line. Multiple line notifications are discouraged by Vaadin (https://vaadin.com/docs/latest/components/notification/#limit-content-length) and should probably be replaced with a dialog.

If you really want a multiple line notification, there is no CSS necessary. Wrap the button in a div.

<cxl-notification>
  <h2>...</h2>
  <div>
    <vaadin-button>...</vaadin-button>
  </div>
</cxl-notification>

---

We should also be using vaadin-icon not iron-icon. iron-icon is deprecated. This was also fixed in my commit.

[saas786]

@saas786 I had it fixed in the commit I pushed.

I understand the h2 is a sibling of vaadin-button. The h2 stretches the content so that it spans multiple lines. This affects the dimensions of the button. The reason the default story doesn't have this issue is because it only spans one line. Multiple line notifications are discouraged by Vaadin (vaadin.com/docs/latest/components/notification/#limit-content-length) and should probably be replaced with a dialog.

If you really want a multiple line notification, there is no CSS necessary. Wrap the button in a div.

<vaadin-notification>
  <h2>...</h2>
  <div>
    <vaadin-button>...</vaadin-button>
  </div>
</vaadin-notification>

I understood your reasoning, and it was working as well, but markup change is not the solution here, so I discarded it. Please work on css change I pushed to branch, and make it Shadow Dom specific selector for <cxl-notification> so vaadin-notification-card should be target.

We should also be using vaadin-icon not iron-icon. iron-icon is deprecated. This was also fixed in my commit.

iron-icon is from live website, its not hard code, so once we swap out iron-icon with vaadin-icon we can update this story.

[anoblet]

Vaadin appends the notification to the document root. There's no way to specify it came from cxl-notification unless we override https://github.com/vaadin/web-components/blob/ae61027c62ffa7f7d70cfc50e43f333addfc74b6/packages/notification/src/vaadin-notification.js#L123.

The closest rule/selector we can get is:

vaadin-notification-card vaadin-button[theme~="icon"]:not([theme~="tertiary-inline"]) {
  flex: none;
}

[pawelkmpt]

@anoblet can we squash a895dd266f26c9fcd49945232c18256ee6ad2918 and 7420f4489120fdebc6f1bb227f1676c734c02498 ?