conversionxl / aybolit

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

Monorepo design #1

Closed lkraav closed 4 years ago

lkraav commented 5 years ago

https://conversionxl.slack.com/archives/C0G52K19U/p1554816779095600

There seems to be confusion about what https://github.com/conversionxl/aybolit monorepo packages is supposed to contain. Let's plan it here.

...
packages/
|- core/
|- cxl-marketing/
   |- scss/
   |- src/
   |- tests/
|- cxl-institute-app/
   |- scss/
   |- src/
   |- tests/
|- cxl-shared/
   |- scss/
   |- src/
|- ...
|- white-label/
chanar commented 5 years ago
...
packages/
|- cxl-components/
   |- scss/
   |- src/
       |- components/
|- cxl-marketing/
   |- scss/
   |- src/
|- cxl-institute-app/
   |- scss/
   |- src/
|- cxl-shared/
   |- scss/
   |- src/
|- cxl-theme/
   |- scss/
   |- src/
|- ...

Where marketing and institute-app contain global css specific to their apps and landing pages. Also, index.js which imports all needed components, vaadin-styles etc.

cxl-component is 1:1 with one of the aybolit packages.

lkraav commented 5 years ago

"Global CSS" - I would think cxl-marketing would be a cxl-marketing-layout (same for cxl-institute-layout) component, not some kind of "global" containerless god object? Because why would we need stuff defined outside of a ...-layout component, or what kind of stuff would such be?

cxl-theme - imports Lumo, then overrides some things?

cxl-shared - not sure what's going to be there (example: media query / device breakpoints...?), but looking forward to the first prototype.

lkraav commented 5 years ago

Preliminary thoughts on https://github.com/conversionxl/aybolit/tree/0e4c3266a69c2cae48a04d96bcc5b6d0eca162f2/packages

  1. upstream has no packages/.../src/templates/ directory, what is its clearly worded purpose description? Example: https://github.com/conversionxl/aybolit/tree/1-monorepo-design/packages/cxl-theme/src/templates

  2. <cxl-marketing-layout> is a component. Why is it sitting outside of packages/cxl-components

  3. packages/cxl-components naming looks suboptimal, this is further confirmed by another similarly named .../components/ sub-directory later. Upstream doesn't say packages/bulma-components, why would we? First thought is packages/cxl, but that could be too generic for our purposes, so perhaps packages/cxl-ui?

  4. ... or how about packages/cxl/{ui,shared,theme}/.../ structure? Might require additional toolchain glob pattern changes, though, and not completely sure of the benefit.

  5. Branches with "fix" commits are damn hard to review. Going through commits bottom up, which is logical to do, reviewer can never tell whether a concept is complete in a commit, or will have more significant changes soon after. @chanar figure out squashing, fast, get consulting for things you don't know. Review time is mega expensive.

  6. Branch contains zero README.md type instructions on how to work with this system. If someone wants to build a custom component, what do they need to import to get cxl-ui looks going, etc? How to work Vaadin overrides?

  7. Why not import @vaadin/vaadin-lumo-styles/all-imports.js instead of granular https://github.com/conversionxl/aybolit/blob/0e4c3266a69c2cae48a04d96bcc5b6d0eca162f2/packages/cxl-theme/src/index.js rationale being "keep it simpler for noobs, at first, optimize later".

  8. Do we need to consider upstream things like https://github.com/conversionxl/aybolit/blob/0e4c3266a69c2cae48a04d96bcc5b6d0eca162f2/packages/core/src/mixins/delegate-focus-mixin.js or will @vaadin elements (most of our base) take care of this for us?

  9. CSS is rather messy: random looking magic numbers all over the place, instead of calc() vs lumo-spacing, etc. Can't tell right now how much is legacy, or useful, and/or passes QA. Font names hardcoded, instead of using theme custom properties. When each component becomes a separate PR, things will hopefully become a much clearer. Means we need to get on that process in a hurry.

  10. We may have to rename to mv {core,white-label} aybolit-{core,white-label}. Because just core sort of implies @conversionxl/core, which makes no sense. :thinking:

  11. ... (maybe more, will edit comment / post follow-up when more found)

lkraav commented 5 years ago

Monday: deliver a clean <cxl-marketing-layout> branch #2

chanar commented 5 years ago
  1. -> https://github.com/conversionxl/aybolit/blob/0e4c3266a69c2cae48a04d96bcc5b6d0eca162f2/packages/cxl-theme/src/templates/lumo-colors.js

Colors and typography need their's to be imported in template file (it didn't work otherwise)

chanar commented 5 years ago
  1. upstream has no packages/.../src/templates/ directory, what is its clearly worded purpose description? Example: https://github.com/conversionxl/aybolit/tree/1-monorepo-design/packages/cxl-theme/src/templates

yes, suggest a better naming. These are template files that create a html template for vaadin style extensions.



$template.innerHTML = `
<dom-module id="lumo-button-edit" theme-for="vaadin-button">
  <template>
    <style>
      ${styles}
    </style>
  </template>
</dom-module>`;

document.head.appendChild($template.content);```
chanar commented 5 years ago
  1. is a component. Why is it sitting outside of packages/cxl-components

This is a marketing "bundle" folder that imports needed styles, components etc. Nothing to do with the component. Somewhere I thought I saw your suggestion for this naming, so I gave this.

chanar commented 5 years ago
  1. packages/cxl-components naming looks suboptimal, this is further confirmed by another similarly named .../components/ sub-directory later. Upstream doesn't say packages/bulma-components, why would we? First thought is packages/cxl, but that could be too generic for our purposes, so perhaps packages/cxl-ui?

Well, cxl-components do contain only components so the naming is clear. cxl-components is a package where within there is components folder containing all components.

cxl-ui feels like components + theme.

Upstream has core which is quite good becose it's only core. For me, component would mean having JS + scss. Instead, the scss is within the theme folders (bulma and bootstrap are themes). That's why not bulma-components. aybolit has setup it's structure towards multiple themes.

  1. ... or how about packages/cxl/{ui,shared,theme}/.../ structure? Might require additional toolchain glob pattern changes, though, and not completely sure of the benefit.

Hmm yes, might require us to edit scripts in our repo and use yarn packages within package. packages/cxl/packages/theme

chanar commented 5 years ago
  1. Branches with "fix" commits are damn hard to review. Going through commits bottom up, which is logical to do, reviewer can never tell whether a concept is complete in a commit, or will have more significant changes soon after. @chanar figure out squashing, fast, get consulting for things you don't know. Review time is mega expensive.

Not sure, I see only one commit with "fix:". I could squash some feat commits but if the commits are long like feat: add cxl-marketing-layout styles and feat: add cxl-marketing-layout package it would be hard to read them.

But yes, will squash stuff that makes sense. i promiseee

chanar commented 5 years ago
  1. Branch contains zero README.md type instructions on how to work with this system. If someone wants to build a custom component, what do they need to import to get cxl-ui looks going, etc? How to work Vaadin overrides?

Makes no sense to me to work on instructions before we even have a proper demo. So far pawel and paul don't have had no problems working with it.

chanar commented 5 years ago

Nice overview of issues @lkraav . Will continue to think along a bit later.

lkraav commented 5 years ago

https://github.com/conversionxl/aybolit/issues/5

Also pushed a very wip branch of weekend's thinking https://github.com/conversionxl/aybolit/commits/leho-1-monorepo-design but it doesn't have much yet. Reviewing your work and continuing my own today.

lkraav commented 4 years ago

Fixed in https://github.com/conversionxl/aybolit/commit/48ff2f803903cb4851e494241ce5a7e48ea1aeb6