zulip icon indicating copy to clipboard operation
zulip copied to clipboard

user_profile_modal: Updates "edit" icon and "copy link" icon of user profile modal.

Open Pratik2026 opened this issue 1 year ago • 20 comments

The PR is made as per the discussion on #CZO

This PR includes the following changes:

  • Updates the font-awesome "edit" icon and "copy-link" icon found in the user profile modal with the corresponding zulip icons.
  • Adds a help center page to document the "copy link to profile" feature.

Screenshots and screen captures:

Light Mode

Screenshot from 2024-04-16 19-55-56

Dark Mode

Screenshot from 2024-04-16 19-49-12

Help-center Page

Screenshot from 2024-03-08 11-45-03

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

Communicate decisions, questions, and potential concerns.

  • [x] 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).

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

Completed manual review and testing of the following:

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

Pratik2026 avatar Feb 24 '24 01:02 Pratik2026

Thanks! Visually, the edit icon looks bigger than the link icon. Can we make them match better?

alya avatar Feb 28 '24 08:02 alya

@alya, I have resized the icon. Let me know if further adjustments are required.

Pratik2026 avatar Feb 28 '24 15:02 Pratik2026

Thanks! Please update the icon in the help center as well:

% git grep "pencil and" help/                  
help/include/manage-this-user-via-user-profile.md:1. Select the **Manage user** tab, or click the **pencil and paper**
help/include/manage-user-tab-tip.md:    You can also access the **Manage user** tab by clicking the **pencil and
help/preview-your-message-before-sending.md:Click the **pencil and paper** (<i class="fa fa-edit"></i>) icon to resume editing.

alya avatar Mar 01 '24 05:03 alya

Are you up for adding a help center page to document the link feature? I would call it "Link to a profile", and place it right under "View someone's profile" in the left sidebar. You can follow the patterns from similar pages for how to do the instructions, and I'll be happy to do an editing pass.

alya avatar Mar 01 '24 06:03 alya

Sure, I am on it!

Pratik2026 avatar Mar 02 '24 08:03 Pratik2026

@alya , I've added a help center page as suggested and also made updates to the icons within the help center pages.

Pratik2026 avatar Mar 03 '24 11:03 Pratik2026

Thank you for the feedback, @alya. I've updated the help center page as suggested.

Pratik2026 avatar Mar 08 '24 06:03 Pratik2026

@alya , Just wanted to know if there is any additional feedback?

Pratik2026 avatar Mar 15 '24 03:03 Pratik2026

Sorry I lost track of this PR! @sahil839 could you please do a review pass?

alya avatar Mar 28 '24 01:03 alya

@sahil839, This PR is ready for another round of review! Please let me know if further changes are required.

Pratik2026 avatar Apr 03 '24 08:04 Pratik2026

@sahil839 just making sure this is still on your list to review.

alya avatar Apr 16 '24 23:04 alya

Posted one comment. Looks good otherwise. Would be good if @karlstolley can also have a look on CSS changes.

sahil839 avatar Apr 17 '24 06:04 sahil839

One improvement can be if the refactor commit can be the first commit, that would remove the unnecessary changes in the commit which changes the icons. You can do this change if that's easy to do, otherwise it is fine.

sahil839 avatar Apr 23 '24 07:04 sahil839

I've removed the formatting changes. Also, Now the refactor commit is the first commit as suggested.

Pratik2026 avatar Apr 23 '24 15:04 Pratik2026

Uhh, sorry, but I still meant to keep the changes for CSS refactroing and updating the icons in separate commits, with only order being changed, which would make it easy to review.

sahil839 avatar Apr 24 '24 02:04 sahil839

My bad, I've now kept the CSS refactoring and updating the icons in separate commits with proper ordering as suggested.

Pratik2026 avatar Apr 24 '24 05:04 Pratik2026

Looks good. @karlstolley pinging you in case you have feedback regarding CSS changes.

sahil839 avatar Apr 24 '24 07:04 sahil839

Overall this LGTM. It would be nice to clean up the underscore-style classes and replace them with hyphenated versions as a prep commit.

karlstolley avatar Apr 24 '24 19:04 karlstolley

As suggested, I've added a prep commit that replaces the underscore-style classes of the user profile modal with the hyphenated one. @karlstolley, please let me know if further adjustments are required.

Pratik2026 avatar Apr 25 '24 15:04 Pratik2026

Those hyphenation prep changes look good, but the commit message should probably be prefixed with "profile_modal" or something more specific than "css," as the message as written doesn't make it clear enough what specifically is being changed.

karlstolley avatar May 01 '24 19:05 karlstolley

The commit message has been updated.

Pratik2026 avatar May 02 '24 14:05 Pratik2026

Commit message looks good, but as a prep commit, that should be your first commit, so that the substantive changes here already have the hyphenated class. git rebase -i is your friend here, and you might have some merge conflicts with yourself that you'll have to fix as you go.

karlstolley avatar May 03 '24 20:05 karlstolley

I've reordered the commits as suggested.

Pratik2026 avatar May 04 '24 06:05 Pratik2026

@timabbott looks like everyone's feedback has been addressed. @timabbott let me know if you need me to re-test.

alya avatar May 06 '24 22:05 alya

I reworked this to address that last batch of feedback and marked this to merge once CI passes, thanks @Pratik2026 and @karlstolley!

timabbott avatar May 07 '24 00:05 timabbott

And @sahil839 . :)

alya avatar May 07 '24 23:05 alya