Skip to content
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

[UI] Remove tick mark referring to third-party agreement #4584

Merged
merged 5 commits into from
Oct 7, 2023
Merged

[UI] Remove tick mark referring to third-party agreement #4584

merged 5 commits into from
Oct 7, 2023

Conversation

ghubrakesh
Copy link
Contributor

@ghubrakesh ghubrakesh commented Oct 3, 2023

Fixes #4583: [UI] remove tickmark from profile

- <div class="flex flex-row gap-4 items-center border-gray-400 border p-4 rounded-lg">
-   <input type="checkbox" name="agree_terms" id="agree_terms" required class="" {% if public_settings %}checked{% endif %}>
-    <label for="agree_terms" class="cursor-pointer w-full">{{_('public_profile_info')}}</label>
- <div>

@Felienne
Copy link
Member

Felienne commented Oct 3, 2023

Thanks @ghubrakesh!! Did you by any chance also do a search of the repo if it is used elsewhere?

@Felienne
Copy link
Member

Felienne commented Oct 4, 2023

Thanks @ghubrakesh!! Did you by any chance also do a search of the repo if it is used elsewhere?

never mind, I did a full search and removed a few more, I think we are now good to go! Thanks again for your first Hedy PR!!

@ghubrakesh
Copy link
Contributor Author

Thanks @ghubrakesh!! Did you by any chance also do a search of the repo if it is used elsewhere?

never mind, I did a full search and removed a few more, I think we are now good to go! Thanks again for your first Hedy PR!!

Oh, I am sorry @Felienne, I did not get the notification for your comment. I hope the issue has been resolved completely by now.

@jpelay jpelay changed the title Update profile.html [UI] Remove tick mark referring to third-party agreement Oct 6, 2023
Copy link
Member

@jpelay jpelay left a comment

Choose a reason for hiding this comment

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

Thank you very much for this PR @ghubrakesh! I left a couple of comments because you accidentaly removed code that is still in use and not the tick mark we wanted! I also attached which lines you have to delete; if you need any further help don't hesitate in contacting us!

Comment on lines 62 to 65
<div class="flex flex-row gap-4 items-center border-gray-400 border p-4 rounded-lg">
<input type="checkbox" name="agree_terms" id="agree_terms" required class="" {% if public_settings %}checked{% endif %}>
<label for="agree_terms" class="cursor-pointer w-full">{{_('public_profile_info')}}</label>
</div>
Copy link
Member

Choose a reason for hiding this comment

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

Can you revert this change, please? This tick mark is used if a user wants their profile to be public, and we don't want to remove this one!

Copy link
Member

Choose a reason for hiding this comment

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

The lines we want to remove are these ones:

hedy/templates/profile.html

Lines 145 to 148 in f7823ac

<div class="flex flex-row items-center mt-4 gap-4">
<input type="checkbox" id="agree_third_party" name="agree_third_party" {% if user_data['third_party'] %}checked{% endif %}>
<label class="text-center" for="agree_third_party">{{_('agree_third_party')}}</label>
</div>

Can you please restore the lines you deleted and delete these ones instead? I can also do this if you want, but I don't have permissions to commit to your branch :c Thank you very much for your help @ghubrakesh!!

Copy link
Member

Choose a reason for hiding this comment

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

See how we're looking to remove the ones that have the string agree_third_party. Wich are present in the lines I attached in 3 ways: the id of the input, the for in the label, and also in this weird string {{_('agree_third_party')}}, which is a label that will later be replaced with the appropriate translation in whichever language the user has set.

The one you removed only has the string: agree_terms which is for agreeing to have a public profile! It's perfectly reasonable to get confused, more so being your first time working with our codebase!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay @jpelay, I will restore the changes made by me (I will add the deleted lines again) but what about the changes made by @Felienne? Should I ignore them and add the lines deleted just by me?

Copy link
Member

Choose a reason for hiding this comment

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

Yes! The changes made by Felienne are correct, and you shouldn't revert them; just the ones I told you about.

Copy link
Member

@jpelay jpelay left a comment

Choose a reason for hiding this comment

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

Perfect! Thank you very much, again, @ghubrakesh! This looks great!

@Felienne
Copy link
Member

Felienne commented Oct 6, 2023

The error we are getting now is unrelated, and hopefully will be fixed soon!

@mergify
Copy link
Contributor

mergify bot commented Oct 6, 2023

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 5fee4d5 into hedyorg:main Oct 7, 2023
8 checks passed
@mergify
Copy link
Contributor

mergify bot commented Oct 7, 2023

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

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.

[UI] remove tickmark from profile
3 participants