Skip to content

[email protected]: Update weatherstack.js #1472

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

naufragoweb
Copy link
Contributor

@tipichris
*Support for free and paid plans
*Automatic day/night icons
*Robust error handling
*Internationalization
*Code optimized

@tipichris 
*Support for free and paid plans
*Automatic day/night icons
*Robust error handling
*Internationalization
*Code optimized
@rcalixte rcalixte marked this pull request as draft May 3, 2025 01:31
@rcalixte
Copy link
Member

rcalixte commented May 3, 2025

@naufragoweb Can you hold off on pull requests that modify this desklet until we have a chance to synchronize on feedback? There are some issues that prevent these from being able to be merged as-is and it would help if you understand why so that you can implement future changes with those changes in mind.

@naufragoweb
Copy link
Contributor Author

@rcalixte
Greetings... Ok, I'm going to suspend pull requests.

@rcalixte
Copy link
Member

rcalixte commented May 3, 2025

Greetings... Ok, I'm going to suspend pull requests.

What you're aiming to do is appreciated but we need to strive for maintainable and readable code. As a minimum, you've deviated from the established whitespace structuring and over-indexed on brevity. I was going to take the time to send this feedback after demonstrating the desired approach with your first pull request that is still open but then came more. 😅

Is this something you can take on or would an example or two help first?

@naufragoweb
Copy link
Contributor Author

What you're aiming to do is appreciated but we need to strive for maintainable and readable code. As a minimum, you've deviated from the established whitespace structuring and over-indexed on brevity. I was going to take the time to send this feedback after demonstrating the desired approach with your first pull request that is still open but then came more. 😅

Thank you for the honest and constructive feedback—I really appreciate you taking the time to guide me on this. You’re absolutely right about prioritizing maintainability and readability, and I apologize for the deviations in whitespace and over-optimization for brevity. That wasn’t my intention, and I’ll make sure to align with the team’s standards moving forward.

If it’s better for the project, I’m completely fine with having my recent pull requests rejected so I can revisit the code with your feedback in mind. I don’t want to create unnecessary work for anyone, and my goal is to contribute helpfully, not disrupt the workflow.

If possible, an example or two of the desired approach would be incredibly helpful, just so I can correct my mistakes more efficiently. But if you’re short on time, I’m happy to adjust things independently based on your notes.

Thanks again for your patience—I’ll strive to improve and match the team’s expectations. Let me know how you’d prefer to proceed.

@rcalixte
Copy link
Member

rcalixte commented May 5, 2025

If it’s better for the project, I’m completely fine with having my recent pull requests rejected so I can revisit the code with your feedback in mind. I don’t want to create unnecessary work for anyone, and my goal is to contribute helpfully, not disrupt the workflow.

Not rejected since the fixes need to happen. We just need to structure it slightly differently. That's why I marked them as a draft. It's a good first pass because it's functional.

If possible, an example or two of the desired approach would be incredibly helpful, just so I can correct my mistakes more efficiently. But if you’re short on time, I’m happy to adjust things independently based on your notes.

I'd started on the first pull request but we can use this one too. This could be a good learning and collaboration opportunity for both of us.

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