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.

Sign up for GitHub

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

Jump to bottom

Avatar tweaks for bots #26301

Closed
wants to merge 1 commit into from
Closed

Avatar tweaks for bots #26301

wants to merge 1 commit into from

Conversation

osamudiamenojo
Copy link
Collaborator

@osamudiamenojo osamudiamenojo commented Jul 20, 2023 •
edited

This PR made changes to the avatars for bot users to make it have a dark indicator at the bottom right as shown in the screenshot below.

Fixes:
#25969

Screenshots and screen captures:
WhatsApp Image 2023-07-19 at 13 36 08
image
WhatsApp Image 2023-07-19 at 13 36 05

Self-review checklist
  • Self-reviewed the changes for clarity and maintainability
    (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • Explains differences from previous plans (e.g., issue description).
  • Highlights technical choices and bugs encountered.
  • Calls out remaining decisions and concerns.
  • Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

  • Each commit is a coherent idea.
  • Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

  • Visual appearance of the changes.
  • Responsiveness and internationalization.
  • Strings and tooltips.
  • End-to-end functionality of buttons, interactions and flows.
  • Corner cases, error conditions, and easily imagined bugs.

@shameondev
Copy link
Member

Hey, @osamudiamenojo! Great job on your first contribution! 🎉 We appreciate your effort and look forward to seeing more of your contributions.

As a first step, I recommend taking a look at the failed CI builds and figuring out how to resolve them. This documentation can be helpful in this case.

Also, keep in mind that you have a good friend who is always ready to assist you - Contributing to Zulip.

@shameondev shameondev added area: bots needs discussion Issue should be discussed on chat.zulip.org before implementation labels Jul 20, 2023
@zulipbot
Copy link
Member

Hello @zulip/server-bots members, this pull request was labeled with the "area: bots" label, so you may want to check it out!

@osamudiamenojo
Copy link
Collaborator Author

Ready for Review @shameondev

@freshpex
Copy link
Collaborator

@zulipbot Ready for review

@amanagr
Copy link
Member

amanagr commented Jul 27, 2023

Thanks for the update. You need to "Squash" all your commits and write a proper commit message as written in https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#commit-summary-part-2.

amanagr
amanagr reviewed Jul 27, 2023
View reviewed changes
web/templates/user_profile_modal.hbs Outdated
style="background-image: url('{{user_avatar}}');">
</div>
</div>
<div class="col-wrap col-right"> <div id="avatar" {{#if user_is_guest}} class="guest-avatar" {{/if}} style="background-image: url('{{user_avatar}}');" {{#if is_bot}} class="bot-avatar" {{/if}}></div></div>
Copy link
Member

Choose a reason for hiding this comment

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

This would be easier to read if the divs were in different lines and the class if conditions were next to each other.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the feedback.
This has been fixed.

@zulipbot zulipbot added size: XL and removed size: M labels Jul 27, 2023
@freshpex freshpex force-pushed the avatar-tweaks-for-bots branch 2 times, most recently from 3b2f21a to ec25baa Compare July 27, 2023 17:04
@freshpex
Copy link
Collaborator

freshpex commented Jul 30, 2023 •
edited

@shameondev @amanagr ready for review
#25969

@sahil839
Copy link
Collaborator

@osamudiamenojo please squash the commits into one as mentioned in the commit guidelines and update the commit message to follow the guidelines so that the PR can be reviewed.

@freshpex freshpex force-pushed the avatar-tweaks-for-bots branch 3 times, most recently from 7c75055 to 2bb47fd Compare August 23, 2023 23:03
@osamudiamenojo
Copy link
Collaborator Author

@sahil839 changes effected. Please review

@sahil839
Copy link
Collaborator

sahil839 commented Aug 28, 2023 •
edited

Ok, now I noticed that we have different width of outlines in different places, so adding the code to app_components.css breaks it. I updated the PR myself to avoid further rounds of updating and review on this with that.

@freshpex I am not sure whether we need to decrease the outline width or not, so I have kept that change as a separate commit for now. I am assuming you intended to decrease the outline because it took much of the space in the avatar shown in message feed for the message sender. Because the outline thickness looks fine to me at other places.
If the problem was only the avatar shown in message feed, we can probably remove the commit to decrease the width of outline, since it was already set to 2px.

@freshpex
Copy link
Collaborator

@sahil839
Yes I intended to decrease the outline because it took much of the space in the avatar shown in message feed for the message sender.

@sahil839
Copy link
Collaborator

Ok, so I removed that commit. Thanks for all your work on this. @timabbott you can review this PR.

@sahil839 sahil839 added the integration review Added by maintainers when a PR may be ready for integration. label Aug 28, 2023
@zulipbot zulipbot added size: M and removed size: XL labels Sep 14, 2023
@freshpex freshpex force-pushed the avatar-tweaks-for-bots branch 3 times, most recently from d35ad05 to adc0056 Compare September 14, 2023 22:24
@osamudiamenojo
Copy link
Collaborator Author

osamudiamenojo commented Sep 15, 2023 •
edited

@timabbott @sahil839 conflicts resolved.

@zulipbot
Copy link
Member

Heads up @osamudiamenojo, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

@freshpex
avatar: Add dark grey indicator for bot users.
8ba3285 
Adds a dark grey indicator to bot users avatar to differentiate
between a bot and a guest user from other users.

Fixes zulip#25969.
@zulipbot
Copy link
Member

Heads up @osamudiamenojo, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

@alya
Copy link
Contributor

alya commented Sep 25, 2023

Thanks for your work on this, @osamudiamenojo ! We have decided to move forward with a different design direction: #26831.

@alya alya closed this Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karlstolley karlstolley karlstolley left review comments

@amanagr amanagr amanagr left review comments

@sahil839 sahil839 sahil839 left review comments

@freshpex freshpex freshpex approved these changes

Assignees
No one assigned
Labels
area: bots has conflicts integration review Added by maintainers when a PR may be ready for integration. needs discussion Issue should be discussed on chat.zulip.org before implementation size: M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@osamudiamenojo @shameondev @zulipbot @freshpex @amanagr @sahil839 @karlstolley @alya