conversionxl / aybolit

Lightweight web components library built with LitElement.
https://conversionxl.github.io/aybolit/
MIT License
7 stars 8 forks source link

feat(cxl-lumo-styles): icon vaadin:globe-wire #263

Closed pawelkmpt closed 1 year ago

github-actions[bot] commented 1 year ago

size-limit report 📦

Path Size
packages/cxl-ui/pkg/dist-web/cxl-ui.js 33.44 KB (+4.05% 🔺)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js 11.87 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js 24.9 KB (+5.51% 🔺)
packages/cxl-ui/pkg/dist-web/vendor.js 125.6 KB (0%)
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 196.97 KB (+1.34% 🔺)
lkraav commented 1 year ago

With every new icon, in theory, there should be a story using it.

I've added more stories recently where small things could be added, so at least could be found with Git blame.

pawelkmpt commented 1 year ago

With every new icon, in theory, there should be a story using it.

I've added more stories recently where small things could be added, so at least could be found with Git blame.

I extended 1c-c story as you suggested @lkraav

I need help with making vaadin icon the same size as lumo. As docs say:

The Vaadin Icons (...) have no safe area around the icon. (...) Vaadin Icons are scaled up and look bigger than the Lumo icons.

Screenshot 2023-02-22 at 11 00 13

lkraav commented 1 year ago

I need help with making vaadin icon the same size as lumo. As docs say:

Hadn't noticed this before.

vaadin-icon uses internal CSS variables for width/height, adjusting these does not help?

pawelkmpt commented 1 year ago

Hadn't noticed this before.

vaadin-icon uses internal CSS variables for width/height, adjusting these does not help?

It uses:

width: var(--lumo-icon-size-m);
height: var(--lumo-icon-size-m);

changing it to --lumo-icon-size-s helps Screenshot 2023-02-22 at 14 49 00

but I'm afraid of changing it on the global level as it will have impact on all vaadin icons.

anoblet commented 1 year ago

You could target just the globe icon with:

[icon="vaadin:globe-wire"] {
  width: var(--lumo-icon-size-xs);
  height: var(--lumo-icon-size-xs);
}

Although I believe we should make all Vaadin icons the same size as the Lumo icons. This could be done with:

[icon^="vaadin:"] {
  width: var(--lumo-icon-size-xs);
  height: var(--lumo-icon-size-xs);
}

https://wshop.fi/eng/css-wildcard-kind-of-css-attribute-selector/

lkraav commented 1 year ago

Although I believe we should make all Vaadin icons the same size as the Lumo icons. This could be done with:

Feels like we should consult on Vaadin Discord before putting in a global rule.

They must have had a reason for building Vaadin Icons like this?

pawelkmpt commented 1 year ago

I will target just he specific CSS class which wraps these items as I need to deliver it. Then we can think about global adjustments.

lkraav commented 1 year ago

I will target just he specific CSS class which wraps these items as I need to deliver it. Then we can think about global adjustments.

image

vaadin-horizontal layout default center alignment suffers if you wrap elements into span-s.

lkraav commented 1 year ago

The Vaadin Icons (...) have no safe area around the icon.

I will target just he specific CSS class which wraps these items as I need to deliver it. Then we can think about global adjustments.

Because vaadin-icon is shipped with box-sizing: border-box, something like padding: var(--lumo-icon-size-m) / 4 gives the best visually equal result to Lumo icons. We recreate their "safe area".

pawelkmpt commented 1 year ago

image

vaadin-horizontal layout default center alignment suffers if you wrap elements into span-s.

I tested with and without <span>. Without it makes equal spacing between all elements and I want to group icon and label together.

Screenshot 2023-02-22 at 16 01 25

Screenshot 2023-02-22 at 16 02 30

pawelkmpt commented 1 year ago

Maybe I should use badge component with "plain" theme or create new component "inline list" based on the badge code 🤔

Lowest on screenshot

Screenshot 2023-02-22 at 16 17 15

lkraav commented 1 year ago

Maybe I should use badge component with "plain" theme or create new component "inline list" based on the badge code thinking

I think easiest and clearest is to just invent a new vaadin-horizontal-layout[theme] value for this structure, like

:host([theme~="cxl-marketing-hero-details"]) {
  ::slotted(span) {
    display: inline-flex;
    align-items: center; /* In Firefox, for some weird reason, things look more center *without* this property. */
  }
}
pawelkmpt commented 1 year ago

I just pushed solution with badges but your idea looks good and maybe even simpler.

lkraav commented 1 year ago

I just pushed solution with badges but your idea looks good and maybe even simpler.

I looked it over, and yes, my idea is clearer with less code.

pawelkmpt commented 1 year ago

Man, this sure ended up a marathon for a few icons!

But I think lots of learnings, and hopefully skill upgrades.

Yeah, way too much time but I definitely learned the way of thinking about components. Thanks for quick responses!

We need more people capable of

  • evaluating frontend at layers this deep
  • getting to an optimal result with less review cycles

Definitely.

There might be no other way than continous practice. Fortunately Vaadin + Lit toolbox has a lot of useful pieces to get elegant solutions almost for whatever we need from our frontend. But need to know the tools.

As with everything. If I knew where vars are placed and what vars we have I'd do it quicker.

lkraav commented 1 year ago

As with everything. If I knew where vars are placed and what vars we have I'd do it quicker.

Maybe, but nobody knows everything.

From that perspective, is there some system we're missing that would've helped? (Other than ChatGPT)

pawelkmpt commented 1 year ago

From that perspective, is there some system we're missing that would've helped? (Other than ChatGPT)

Complete list of CSS vars and files they're defined. Chrome dev tools help but I'm not sure if they show all of them.