AOSSIE-Org / Resonate

Clubhouse, but Open Source. A social voice platform.
GNU General Public License v3.0
171 stars 123 forks source link

#230 fix | used Theme to give dynamic colors #297

Closed MrBooring closed 1 month ago

MrBooring commented 3 months ago

Description

This PR addresses issues with color inconsistency in several key areas of the app, including the Email ID, Password, and "New to Resonate" text in emails, the logout icon in the profile section, and the visibility of the "verify" text in verification emails. The proposed solution involves implementing a unified theme throughout the entire application. Upon approval, I estimate that this task can be completed within 1-2 days.

Fixes #230, Closes #318, #316

Type of change

How Has This Been Tested?

To verify the changes, I conducted manual testing by following these steps:

Changed the theme settings to both dark mode and light mode.

  1. Email ID and Password fields in login page.
  2. "New to Resonate" text in login page.
  3. Logout icon in the profile section.
  4. Visibility of the "verify your email" text in verification emails.

Checked if all texts and icons were clearly visible and legible in both dark and light modes. To reproduce the testing process:

Navigate to the settings or preferences section of the application. Locate the theme settings and switch between dark mode and light mode. Access the areas mentioned above, such as the login page for Email ID and Password fields, emails containing the "New to Resonate" text, the profile section for the logout icon, and verification emails for the "verify" text. Ensure that all texts and icons maintain visibility and legibility in both dark and light modes. Please let me know if you need further clarification or assistance with testing.

included screenshots below

Screenshot 2024-03-13 022539

Screenshot 2024-03-13 022654

Screenshot 2024-03-13 022829

Checklist:

Maintainer Checklist

Aarush-Acharya commented 1 month ago

Hey @MrBooring, Good Work πŸš€βœ¨

There are some conflicts please do resolve them correctly and push back

I reviewed your code it is good to merge but please do take care of the following as well

On the verify email_verification_screen there is a

https://github.com/AOSSIE-Org/Resonate/assets/92685647/d4f54550-d1c9-4cb4-84cb-a951f2f849b5

Rn the colour of the animated container and the text in it along with the parent container wrapping the other widgets is not set so that they are clearly visible in both dark and light themes

Play with their colours such that they are visible and look good on both the light and dark modes hence enhancing the UI

Push after making the suggested changes and resolving the conflicts carefully achieving a clean state, will merge this PR once the ones made prior to this are resolved

MrBooring commented 1 month ago

Sure ill look into the mentioned conflicts and try to complete it by tomorrow itself

Get Outlook for iOShttps://aka.ms/o0ukef


From: Aarush Acharya @.> Sent: Friday, May 10, 2024 8:39:58 PM To: AOSSIE-Org/Resonate @.> Cc: Siddhant Vedpathak @.>; Mention @.> Subject: Re: [AOSSIE-Org/Resonate] #230 fix | used Theme to give dynamic colors (PR #297)

Hey @MrBooringhttps://github.com/MrBooring, Good Work πŸš€βœ¨

There are some conflicts please do resolve them correctly and push back

I reviewed your code it is good to merge but please do take care of the following as well

On the verify email_verification_screen there is a

https://github.com/AOSSIE-Org/Resonate/assets/92685647/d4f54550-d1c9-4cb4-84cb-a951f2f849b5

Rn the colour of the animated container and the text in it along with the parent container wrapping the other widgets is not set so that they are clearly visible in both dark and light themes

Please play with their colours such that they are visible, look good on both the light and dark modes hence enhancing the UI

Push after making the suggested changes and resolving the conflicts carefully achieving a clean state, will merge this PR once the ones made prior to this are resolved

β€” Reply to this email directly, view it on GitHubhttps://github.com/AOSSIE-Org/Resonate/pull/297#issuecomment-2104779304, or unsubscribehttps://github.com/notifications/unsubscribe-auth/A2MUTBQAKLPF3WZY3SD4FF3ZBTPMNAVCNFSM6AAAAABES7MDZ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBUG43TSMZQGQ. You are receiving this because you were mentioned.Message ID: @.***>

MrBooring commented 1 month ago

@Aarush-Acharya am sorry I was not able to work i got caught up in some other work well, I looked at the page you mentioned and I didn't understand why we have that feature, when the user clicks on that headline it disappears.

this is before

Screenshot_1716063111

and this is after

Screenshot_1716062955

should I remove the animation where it disappears?

Aarush-Acharya commented 1 month ago

@MrBooring no do not remove it, change the background colours of the containers visible when the animation happens it forms an enclosed circle the circle is not visible as the colour is set to white, please adjust the colours of the containers so that they are visible in both dark and white themes

MrBooring commented 1 month ago

@Aarush-Acharya flutter_01 flutter_02

please check this

Aarush-Acharya commented 1 month ago

Yes exactly like this good work,

Just make some more minor changes that are recently suggested and will merge it, good going

these are some additional features

Also take care of the merge conflicts while doing so and if it is possible do it by today these are very minor changes would merge this PR today itself

P.S. You have implemented the part where you have made the logout button colour to adapt with the app colour, there are two more duplicate PR doing the same thing so once this is merged i will close both of them as well. You were given the priority as you made the PR before them

MrBooring commented 1 month ago

Sure ill look into it tonight

Get Outlook for iOShttps://aka.ms/o0ukef


From: Aarush Acharya @.> Sent: Friday, May 24, 2024 7:03:47 PM To: AOSSIE-Org/Resonate @.> Cc: Siddhant Vedpathak @.>; Mention @.> Subject: Re: [AOSSIE-Org/Resonate] #230 fix | used Theme to give dynamic colors (PR #297)

Yes exactly like this good work,

Just make some more minor changes and will merge it, good going

these are some additional features

β€” Reply to this email directly, view it on GitHubhttps://github.com/AOSSIE-Org/Resonate/pull/297#issuecomment-2129549384, or unsubscribehttps://github.com/notifications/unsubscribe-auth/A2MUTBSV4WQOH6YWCOWF5JDZD46TXAVCNFSM6AAAAABES7MDZ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRZGU2DSMZYGQ. You are receiving this because you were mentioned.Message ID: @.***>

MrBooring commented 1 month ago

flutter_03

@Aarush-Acharya I have completed those changes as requested. hope they align with the suggestions and I've resolved the conflict too. Thank you for a chance to contribute 😊

Aarush-Acharya commented 1 month ago

hey @MrBooring thank you for the contribution, but it seems like you have missed the changes i requested as reviews to the code

Please implement them and push again, lets go πŸš€βœ¨

MrBooring commented 1 month ago

I've made 4 commits

  1. The original pr
  2. container having text "verify your email" color issue and dynamic height
  3. OTP fields
  4. Merge conflicts

other than this have I missed something

Get Outlook for iOShttps://aka.ms/o0ukef


From: Aarush Acharya @.> Sent: Saturday, May 25, 2024 10:59:18 AM To: AOSSIE-Org/Resonate @.> Cc: Siddhant Vedpathak @.>; Mention @.> Subject: Re: [AOSSIE-Org/Resonate] #230 fix | used Theme to give dynamic colors (PR #297)

hey @MrBooringhttps://github.com/MrBooring thank you for the contribution but this is just one of the suggested changes what about the others, please do complete all of them and push again

β€” Reply to this email directly, view it on GitHubhttps://github.com/AOSSIE-Org/Resonate/pull/297#issuecomment-2130785846, or unsubscribehttps://github.com/notifications/unsubscribe-auth/A2MUTBRAV6U2DQOL5DDXNYDZEAOS5AVCNFSM6AAAAABES7MDZ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZQG44DKOBUGY. You are receiving this because you were mentioned.Message ID: @.***>

Aarush-Acharya commented 1 month ago

@MrBooring yes, please do visit this PR on Github i believe it is because you are interacting via outlook you are not able to see the review changes requested in the form of comments on the code lines

Please view the PR on github.com for further interactions and once you are on this PR please look above my otp field comment to view the requested changes as comments on code

MrBooring commented 1 month ago

@Aarush-Acharya now I am viewing the PR on github.com I didn't see any comments though yesterday when I was resolving the merge conflict I found one comment to use constant height to make it dynamic and I did that part, if there are other things please help me to find it

Aarush-Acharya commented 1 month ago

Hey @MrBooring sincere apologies I had not submitted the review it was still in the Draft my bad, please check again,

Please do get this done so that we merge it tonight only

Aarush-Acharya commented 1 month ago

Hey @MrBooring you there ??

MrBooring commented 1 month ago

@Aarush-Acharya sorry am currently out right now i will work on it as soon as i get home sorry for the delay

MrBooring commented 1 month ago

@Aarush-Acharya I've completed the review changes requested. if you have any further changes please do let me know

flutter_01 flutter_02

Aarush-Acharya commented 1 month ago

Good work, @MrBoorin, it looks real good and aligns with what I envisioned, I believe you missed looking into the Post Script of a review comment I made, please implement that as well Apart from that everything feels clean and good

MrBooring commented 1 month ago

@Aarush-Acharya sorry I missed that one, my bad. I've completed that too

Aarush-Acharya commented 1 month ago

Great οΌ·ork @MrBooring, I am merging this Pull Request Now

MrBooring commented 1 month ago

@Aarush-Acharya happy to help 😊