feat: add web component library with Stencil.js #92
Conversation
size-limit report 📦
|
rschlaefli
changed the title
| @@ -0,0 +1,3 @@ | |||
| :host { | |||
There was a problem hiding this comment.
Can we get rid of the .css files as we do everything in Tailwind and as they contain just one and always the same display: block? E.g., can we move that single statement to Tailwind too?
There was a problem hiding this comment.
maybe we can add the in React as a wrapper as done in some components? or is that for something else?
| styleUrl: "tc-job-listing.css", | ||
| shadow: true, | ||
| }) | ||
| export class JobListing { |
There was a problem hiding this comment.
We should make these names more generic. Is it really just a job listing or can it be used for any listing with an image and tags and a description?
Also, please remove the tc prefix from the file names :)
| render() { | ||
| return ( | ||
| <div | ||
| class={{ |
There was a problem hiding this comment.
why are there sometimes classnames passed like this instead of as a string? -> use a single string for consistency
There was a problem hiding this comment.
I think in this case here the class names should be passed normally. I think there are cases where this type of syntax makes sense
const textSection = ( <div class={{ "flex-1": true, "order-2": true, // Text always second on small screens "md:order-2": this.imageOnLeft, // Order on medium screens "md:order-1": !this.imageOnLeft, // Order on medium screens "px-4": true, "font-sans": true, "font-normal": true, }} >
There was a problem hiding this comment.
good point :) to get this kind of dynamic merging of classes depending on some props, use the https://www.npmjs.com/package/tailwind-merge utility (as shown in the readme there)
e.g., class={twMerge('flex-1 order-2 md:order-1 px-4 font-sans font-normal', this.imageOnLeft && 'md:order-2')}
so if this.imageOnLeft is true, md:order-2 will override md:order-1. if it is false, it will be ignored
packages/tc-web/src/components/tc-grid-container/tc-grid-container.tsx
Outdated
Show resolved
Hide resolved
packages/tc-web/src/components/tc-grid-container/tc-grid-container.tsx
Outdated
Show resolved
Hide resolved
| export class TcGridContainer { | ||
| @Prop() mdColumns: number = 2; | ||
| @Prop() lgColumns: number = 4; | ||
| @Prop() columns: number = 1; |
| @Prop() mdColumns: number = 2; | ||
| @Prop() lgColumns: number = 4; | ||
| @Prop() columns: number = 1; | ||
| @Prop() gap: string = ""; |
There was a problem hiding this comment.
maybe like
| @Prop() gap: string = ""; | |
| @Prop() gap: 'sm' | 'md' | 'lg' | 'xl' = "md"; |
using a default of md to cover the "default case" in the switch statement (so that default case could be removed now)
| <h4 class="!m-0 text-xl md:text-3xl font-semibold"> | ||
| {this.colTitle} | ||
| </h4> | ||
| <div class="mt-2 text-lg">{this.description}</div> |
There was a problem hiding this comment.
should we hide this if there is no description set? otherwise we have an empty space (mt-2)
| }) | ||
| export class ImgQuoteCard { | ||
| @Prop() quote: string; | ||
| @Prop() quotee: string; |
There was a problem hiding this comment.
some of the prop names are really hard to identify what it is used for. could you improve naming (does not matter if the prop names are a bit longer, quoteLabel or so?)
| shadow: true, | ||
| }) | ||
| export class SegmentContainer { | ||
| @Prop() segTitle: string; |
There was a problem hiding this comment.
| @Prop() segTitle: string; | |
| @Prop() title: string; |
the component is called SegmentContainer -> title would suffice to make it clear what it is
There was a problem hiding this comment.
hab es geändert, nun kann man es aufrufen mit title
@prop({attribute:"title"}) segTitle: string;
Wieso?
title is a reserved name
There was a problem hiding this comment.
hoi @Queentaker, ich glaube, title ist ein html attribut für accessibility. dürftest du dann wohl nicht als prop name verwenden. sorry, das wusste ich nicht, dann müsste man es wohl segmentTitle nennen (dann ist der name auch klar)
No description provided.