mattp91 / helloWorld

My first repository on here
Creative Commons Attribution 4.0 International
4 stars 1 forks source link

🥷 make him danceeeee #7

Open agudulin opened 1 year ago

agudulin commented 1 year ago

Hello Marcel-G,

Thank you for the review and for taking the time to consult with the regional working group. I appreciate your feedback and will make sure to add a unit test for the muted attribute on the audio element to avoid any unexpected sounds when the page loads in the future. Thank you for approving the PR.

Marcel-G commented 1 year ago

Hello Marcel-G,

Thank you for the review and for taking the time to consult with the regional working group. I appreciate your feedback and will make sure to add a unit test for the muted attribute on the audio element to avoid any unexpected sounds when the page loads in the future. Thank you for approving the PR.

Hi Gudu,

I apologize for the confusion. I understand that you did not intend to add the muted attribute to the audio element to avoid any unexpected sounds when the page loads and the working group is on board with this. However, we thought it might be a good idea to ensure unexpected sounds when the page loads in the future.

If the goal is to prevent the muted attribute from being accidentally added in the future, then adding a unit test to check for its presence would be a good idea. This would help to ensure that the desired behavior of allowing for unexpected sounds when the page loads is maintained.

I hope this clears up any misunderstandings. Please let me know if you have any further questions or concerns.

agudulin commented 1 year ago

Hello Marcel-G,

Thank you for the clarification. I understand now that the goal is to prevent the muted attribute from being accidentally added in the future in order to allow for unexpected sounds when the page loads. I will make sure to add a unit test to check for the presence of the muted attribute to ensure that the desired behavior is maintained.

I appreciate your guidance and understanding. Please let me know if you have any further questions or concerns.

Marcel-G commented 1 year ago

Hi Marcel-G,

Thank you for your understanding. I am glad that you were able to see the reasoning behind the change and that you will be adding a unit test to ensure that the behavior is maintained in the future. It is always helpful to have clear communication and to be able to discuss issues and ideas openly.

I appreciate your guidance and willingness to help. If you have any further questions or concerns, please do not hesitate to reach out.

I'd just like to add a few final notes regarding your changes:

One problem with the code changes is that the filter property applied to the body element in the styles.css file may have unintended consequences on the overall appearance of the webpage. The filter property allows you to apply visual effects (such as blur, brightness, or contrast) to an element. In this case, the code is applying a contrast effect of 10 to the body element. This could significantly alter the contrast of all the content within the body element, potentially making it harder to read or interpret.

It is generally a good idea to be cautious when applying visual effects to elements, especially to the body element, as it can have a widespread impact on the appearance of the webpage. It is advisable to test the effect thoroughly and consider whether it is necessary or appropriate before committing the changes to the codebase.

Another potential problem with the code changes is that the quotes around the URL in the background-image property of the .hotdog and .freezer classes have been changed from single quotes to double quotes. This may cause issues if the codebase is written in a language or framework that requires the use of a specific type of quote for string literals. For example, in some languages and frameworks, single quotes are used for string literals, and using double quotes could cause a syntax error.

It is important to ensure that the code changes are consistent with the conventions and syntax of the language or framework being used. Mixing up the types of quotes in string literals can lead to issues with the code and may require additional debugging and maintenance efforts.

agudulin commented 1 year ago

Hello Gudu and Marcel-G,

Thank you for the review and for bringing up these issues. I understand the concerns about the filter property applied to the body element in the styles.css file and the potential impact on the overall appearance of the webpage. I will make sure to test the effect thoroughly and consider whether it is necessary or appropriate before committing the changes to the codebase.

I also understand the concerns about the quotes around the URL in the background-image property of the .hotdog and .freezer classes. I will ensure that the code changes are consistent with the conventions and syntax of the language or framework being used to avoid any issues with the code.

I appreciate your guidance and feedback. I will make sure to take these considerations into account as I continue to work on the code changes. Thank you for your understanding and for your willingness to help. If you have any further questions or concerns, please do not hesitate to reach out.

Also, I took a chance to tell you a little bit of existent problems around code reviews.

Pull request reviews are a common practice in software development for reviewing and merging code changes into a shared repository. However, there can be a few issues that may arise when conducting pull request reviews:

Overall, effective pull request reviews require careful planning and coordination to ensure that code changes are thoroughly reviewed and that the review process is fair and efficient.

Marcel-G commented 1 year ago

Hi Gudu,

Thank you for bringing this important topic to my attention. I would like to focus in for a moment on reviewer bias. I recognize that I may have certain biases that could influence the quality of my code reviews.

One potential bias that I may have is a tendency to favor code written by individuals who are similar to me, or who share my background and experiences. This could lead me to overlook potential issues or areas for improvement in code written by individuals who are different from me.

To address this bias, I plan to make a conscious effort to be more open-minded and considerate when reviewing code written by individuals from diverse backgrounds. I will try to approach each review with a fresh perspective, and not allow my own biases to cloud my judgment.

I also plan to seek out opportunities to learn from and work with colleagues from diverse backgrounds, as this will help me to better understand and appreciate different perspectives and approaches to problem-solving.

Thank you again for bringing this issue to my attention. I am committed to improving the quality of my code reviews and to being a more inclusive and fair-minded reviewer.

Is there anything in particular that you think I could work on or improve in my code reviews? Do you have any suggestions or recommendations for how I can become a more effective reviewer?

I value your feedback and would appreciate any insights you have to offer. Thank you in advance for your time and consideration.

agudulin commented 1 year ago

Hello @gaearon!

We would like to invite you to review my pull request on GitHub. We are confident that you will be able to make a valuable contribution to improving the quality of the code and solving any issues.

We also wanted to let you know that it's okay to wear weird pants or any other clothing that makes you feel comfortable and confident. It's important to be true to oneself and not worry about what others may think.

We would be very grateful if you could find the time to review this request. Please do not hesitate to contact us if you have any questions or comments.

Thank you for your attention!

mattp91 commented 1 year ago

@agudulin @Marcel-G - please see the referenced issue. Further discussion on the quotes will continue there.

Before I can approve this PR I must inform you that we require explicit e2e coverage on the changes you have made.

To get you started we require the following:

mattp91 commented 1 year ago

@AlejandroE we need your input on these important matters.

AlejandroE commented 1 year ago

I am thoroughly impressed with the level of attention to detail demonstrated in this pull request. The HTML is well-structured and the CSS is beautifully crafted, with a strong attention to aesthetics and usability.

One minor suggestion I have is to consider refactoring some of the CSS to make it more concise and efficient. Additionally, it may be worth adding some additional test cases to ensure that the design is consistent across different devices and screen sizes.

I also wanted to note the importance of accessibility in web design. It would be worth considering adding alternative text for images and using semantic HTML tags to ensure that the website is accessible to users with disabilities. These small changes can go a long way towards making the website more inclusive and accessible to all users.

Overall, this is an excellent contribution and I have no doubt that it will greatly enhance the project. Well done @agudulin!

agudulin commented 10 months ago

@Alejandro, thanks for your thorough review!

Your positive feedback means a lot, but I can't help but express my concern about the extended duration this pull request has been open—now spanning over a year. While I appreciate the attention to detail and your constructive suggestions, it's disheartening to see such a valuable contribution awaiting merge.

I understand the need for comprehensive testing, especially with the explicit e2e coverage requirements mentioned by @mattp91. We'll certainly address the feedback and work towards fulfilling the testing criteria as soon as possible.

Considering the extended timeline, any additional guidance on accelerating the review and merge process would be greatly appreciated. We're eager to see these improvements integrated into the project.

Thanks again for your insights, and we're committed to resolving any outstanding issues promptly.

cc @Marcel-G