Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the SiWeatherWidgetComponent, a responsive weather display component featuring vertical and horizontal layouts, signal-based inputs, and container queries for adaptive styling. It also updates TypeScript configurations to ignore version 5.0 deprecations. Feedback focuses on adhering to the project's Angular and UX style guides, specifically by removing redundant standalone properties, using computed signals for derived host classes, avoiding abbreviations in UI text, and ensuring proper sentence-case capitalization.
I am having trouble creating individual review comments. Click here to see my feedback.
projects/element-ng/weather-widget/lib/si-weather-widget.component.ts (30)
In Angular (v17+), components are standalone by default. Please remove the redundant standalone: true property from the @Component decorator.
References
- In Angular (v17+), components are standalone by default, so standalone: true is not required in the @component decorator for a component to be considered standalone.
projects/element-ng/weather-widget/lib/si-weather-widget.component.ts (36)
The repository style guide recommends using computed() for derived state. The host class is derived from the layout input. Using a computed signal for this would align better with the project's architectural patterns.
First, import computed from @angular/core.
Then, add a computed signal to your component class:
readonly hostClasses = computed(() => `weather-widget weather-widget--${this.layout()}`);Finally, use this new signal in the host binding.
'[class]': 'hostClasses()'
References
- Use computed() for derived state. (link)
projects/element-ng/weather-widget/lib/si-weather-widget.component.html (29)
According to the UX Writing Guidelines, abbreviations should be avoided. Please use 'Precipitation' instead of 'Precip'.
<span class="text-secondary">Precipitation</span>
References
- Avoid abbreviations (info, incl, excl) and acronyms. (link)
projects/element-ng/weather-widget/lib/si-weather-widget.component.html (41)
According to the UX Writing Guidelines, you should only capitalize the first letter of the first word in UI text. Please change 'Feels Like' to 'Feels like'.
<span class="text-secondary">Feels like</span>
References
- Capitalize only the first letter of the first word in titles, tooltips, menu items, list items, and buttons. (link)
|
@panch1739 Hey awesome - your first vibe coded Element component 🚀 👏 It looks like it still created too much custom CSS instead of using our classes and stuff. With a couple prompts this should be fixable as well as any build errors. Then we miss the playground examples which would be another prompt. And some unit tests. I could think that when you'd have prompted this directly in the element repo, it would've likely done it automatically because it would've picked up the structure. But since you did it with the Element Agent, library development is probably not what it is tuned for, right @Killusions ? |
This PR is a test for the weather widget. @timowolf @kfenner I have no idea what im doing. I was also issued some errors that i dont know how to solve 🤷
Im not 100% sure they are related to the weather widget at all