Skip to content

feat(weather-widget): add weather widget#2021

Draft
panch1739 wants to merge 1 commit intomainfrom
feat/weather-widget
Draft

feat(weather-widget): add weather widget#2021
panch1739 wants to merge 1 commit intomainfrom
feat/weather-widget

Conversation

@panch1739
Copy link
Copy Markdown
Member

@panch1739 panch1739 commented May 6, 2026

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

Screenshot 2026-05-06 at 17 35 43

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

medium

In Angular (v17+), components are standalone by default. Please remove the redundant standalone: true property from the @Component decorator.

References
  1. 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)

medium

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
  1. Use computed() for derived state. (link)

projects/element-ng/weather-widget/lib/si-weather-widget.component.html (29)

medium

According to the UX Writing Guidelines, abbreviations should be avoided. Please use 'Precipitation' instead of 'Precip'.

      <span class="text-secondary">Precipitation</span>
References
  1. Avoid abbreviations (info, incl, excl) and acronyms. (link)

projects/element-ng/weather-widget/lib/si-weather-widget.component.html (41)

medium

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
  1. Capitalize only the first letter of the first word in titles, tooltips, menu items, list items, and buttons. (link)

@kfenner
Copy link
Copy Markdown
Member

kfenner commented May 7, 2026

@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 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants