feat(cxl-ui): cxl-featured-image add component #413

anoblet · 2024-05-15T17:50:17Z

https://app.clickup.com/t/86b0647n0

github-actions · 2024-05-15T17:51:47Z

size-limit report 📦

Path	Size
packages/cxl-ui/pkg/dist-web/cxl-ui.js	44.2 KB (+1.15% 🔺)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js	11.89 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js	28.99 KB (0%)
packages/cxl-ui/pkg/dist-web/vendor.js	138.23 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-institute.js, 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, packages/cxl-ui/pkg/dist-web/manifest.js, packages/cxl-ui/pkg/dist-web/unresolved.js, packages/cxl-ui/pkg/dist-web/vendor.js	275.8 KB (+0.19% 🔺)

packages/cxl-ui/scss/cxl-featured-image.scss

pawelkmpt · 2024-05-16T09:35:49Z

packages/cxl-ui/scss/cxl-featured-image.scss

+:host {
+  position: relative;
+  display: block;
+  min-width: 360px;


Suggested change

min-width: 360px;

min-width: 320px;

At the same time I'm wondering whether it even needs min-width. It's block element which means it's 100% wide by default. And on screens smaller than 360 it causes horizontal scrollbar.

Since the element technically doesn't have any content(only a background), without setting a minimum width, it collapses to 0.

pawelkmpt · 2024-05-16T09:37:54Z

One more thing, it should be visible on mobile only. Now question is whether it's better to encapsulate visibility inside the component or control it with external CSS. I don't know the answer yet

anoblet · 2024-05-16T14:13:21Z

One more thing, it should be visible on mobile only. Now question is whether it's better to encapsulate visibility inside the component or control it with external CSS. I don't know the answer yet

The correct way would be to do it from the outside, though it wouldn't be a big deal taking a shortcut here.

pawelkmpt · 2024-06-13T08:58:33Z

packages/cxl-ui/scss/cxl-featured-image.scss

+:host {
+  position: relative;
+  display: block;
+  min-width: 320px;


I'm thinking min-width is unnecessary. At least component placed on the real WP page looks good without it.

pawelkmpt · 2024-06-13T09:02:21Z

packages/cxl-ui/scss/cxl-featured-image.scss

+    bottom: 0;
+    width: 100%;
+    height: 75%;
+    background-color: var(--cxl-featured-image-bg-color, var(--cxl-brand-color-black, rgb(26, 26, 26)));


We do not have --cxl-featured-image-bg-color defined anywhere so component background color is not compatible with what we see on the desktop.

What about refactoring this code:

aybolit/packages/cxl-ui/scss/cxl-section.scss

Lines 119 to 147 in 22f6225

:host(.wide-background-black) {

&::before {

background-color: var(--cxl-brand-color-black);

}

}

:host(.wide-background-red) {

&::before {

background-color: var(--lumo-primary-color);

}

}

:host(.wide-background-blue) {

&::before {

background-color: var(--cxl-brand-color-blue);

}

}

:host(.wide-background-light-blue) {

&::before {

background-color: var(--cxl-brand-color-light-blue);

}

}

:host(.wide-background-green) {

&::before {

background-color: var(--cxl-brand-color-green);

}

}

We could set variable value instead of explicit background color value. Doable @anoblet?

pawelkmpt · 2024-06-13T09:57:55Z

One more thing needed

export { CXLFeaturedImage } from './components/cxl-featured-image';

anoblet · 2024-06-13T14:07:02Z

Added export
Kept min-width since without it, the component won't show in Storybook(Let me know if we really want to remove it)
Set a default image source
Added media query to hide on desktop
Added class based background color system(

aybolit/packages/cxl-ui/scss/cxl-featured-image.scss

Line 35 in fa456ce

:host(.black) {

). Let me know if this is the correct strategy.

pawelkmpt

Good approach. Just two things to fix. Please also squash all commits into one

pawelkmpt · 2024-06-13T19:43:20Z

packages/cxl-ui/scss/cxl-featured-image.scss

+
+:host(.red) {
+  #container {
+    background-color: var(--cxl-brand-color-red);


Suggested change

background-color: var(--cxl-brand-color-red);

background-color: var(--lumo-primary-color);

pawelkmpt · 2024-06-13T19:46:03Z

packages/cxl-ui/src/components/cxl-featured-image.js

+  @property({ type: String }) src =
+    'https://ebpyu8ye7bj.exactdn.com/institute/wp-content/uploads/2023/12/ben-labay_team_headshot-1x1-bw-min-fix-min-compressed.png?strip=all&lossy=1&fit=720%2C720&ssl=1';


No need to set default image. Especially with such URL. Keep it empty.

https://app.clickup.com/t/86b0647n0

anoblet requested review from lkraav and pawelkmpt May 15, 2024 17:50

pawelkmpt requested changes May 16, 2024

View reviewed changes

anoblet requested a review from pawelkmpt May 16, 2024 14:13

pawelkmpt requested changes Jun 13, 2024

View reviewed changes

anoblet requested a review from pawelkmpt June 13, 2024 14:08

pawelkmpt requested changes Jun 13, 2024

View reviewed changes

anoblet force-pushed the anoblet/feat/featured-image branch from fa456ce to ac0ca62 Compare June 13, 2024 21:15

anoblet requested a review from pawelkmpt June 13, 2024 21:15

feat(cxl-ui): cxl-featured-image add component

b3de6c6

https://app.clickup.com/t/86b0647n0

pawelkmpt force-pushed the anoblet/feat/featured-image branch from ac0ca62 to b3de6c6 Compare June 20, 2024 04:15

pawelkmpt approved these changes Jun 21, 2024

View reviewed changes

pawelkmpt merged commit d0a004e into master Jun 21, 2024
5 checks passed

pawelkmpt deleted the anoblet/feat/featured-image branch June 21, 2024 09:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cxl-ui): cxl-featured-image add component #413

feat(cxl-ui): cxl-featured-image add component #413

anoblet commented May 15, 2024

github-actions bot commented May 15, 2024 •

edited

Loading

pawelkmpt May 16, 2024

anoblet May 16, 2024 •

edited

Loading

pawelkmpt commented May 16, 2024

anoblet commented May 16, 2024

pawelkmpt Jun 13, 2024

pawelkmpt Jun 13, 2024

pawelkmpt commented Jun 13, 2024

anoblet commented Jun 13, 2024 •

edited

Loading

pawelkmpt left a comment

pawelkmpt Jun 13, 2024

pawelkmpt Jun 13, 2024

	:host(.wide-background-black) {
	&::before {
	background-color: var(--cxl-brand-color-black);
	}
	}

	:host(.wide-background-red) {
	&::before {
	background-color: var(--lumo-primary-color);
	}
	}

	:host(.wide-background-blue) {
	&::before {
	background-color: var(--cxl-brand-color-blue);
	}
	}

	:host(.wide-background-light-blue) {
	&::before {
	background-color: var(--cxl-brand-color-light-blue);
	}
	}

	:host(.wide-background-green) {
	&::before {
	background-color: var(--cxl-brand-color-green);
	}
	}

		@property({ type: String }) src =
		'https://ebpyu8ye7bj.exactdn.com/institute/wp-content/uploads/2023/12/ben-labay_team_headshot-1x1-bw-min-fix-min-compressed.png?strip=all&lossy=1&fit=720%2C720&ssl=1';

feat(cxl-ui): cxl-featured-image add component #413

feat(cxl-ui): cxl-featured-image add component #413

Conversation

anoblet commented May 15, 2024

github-actions bot commented May 15, 2024 • edited Loading

size-limit report 📦

pawelkmpt May 16, 2024

Choose a reason for hiding this comment

anoblet May 16, 2024 • edited Loading

Choose a reason for hiding this comment

pawelkmpt commented May 16, 2024

anoblet commented May 16, 2024

pawelkmpt Jun 13, 2024

Choose a reason for hiding this comment

pawelkmpt Jun 13, 2024

Choose a reason for hiding this comment

pawelkmpt commented Jun 13, 2024

anoblet commented Jun 13, 2024 • edited Loading

pawelkmpt left a comment

Choose a reason for hiding this comment

pawelkmpt Jun 13, 2024

Choose a reason for hiding this comment

pawelkmpt Jun 13, 2024

Choose a reason for hiding this comment

github-actions bot commented May 15, 2024 •

edited

Loading

anoblet May 16, 2024 •

edited

Loading

anoblet commented Jun 13, 2024 •

edited

Loading