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

A basic implementation of user avatar pill #9845

Closed
wants to merge 3 commits into from

Conversation

shubham-padia
Copy link
Member

@shubham-padia shubham-padia commented Jun 27, 2018

Fixes #9842 .

Result:
selection_169

User group settings:
selection_168

@rishig @maxnuss I've implemented the user avatar pills as above.

@maxnuss
Copy link
Contributor

maxnuss commented Jun 28, 2018

Thanks @shubham-padia! I wouldn't change anything about the styling here; I think these look great. Should be ready to go!

@timabbott
Copy link
Sponsor Member

The padding/margins at the top are a bit off on the pills (most visible if you click on them so you see the green):

image

Otherwise this lgtm.

@rishig
Copy link
Member

rishig commented Jul 1, 2018

I could also imagine rounding all four corners of the avatar?

@shubham-padia
Copy link
Member Author

The padding/margins at the top are a bit off on the pills

@timabbott What browser are you using, I'm unable to repro this.
Chrome:
selection_172

Firefox:
selection_171

@shubham-padia
Copy link
Member Author

shubham-padia commented Jul 1, 2018

I could also imagine rounding all four corners of the avatar?

Here's a preview:
selection_173

@rishig I personally like the pills without the right borders, what are your thoughts after viewing the preview ?
I'd open a feedback topic on #design otherwise.

@timabbott
Copy link
Sponsor Member

@shubham-padia I think it's a zoom issue; I see it at 110% zoom in Chrome, but not other sizes.

@maxnuss
Copy link
Contributor

maxnuss commented Jul 5, 2018

I can take a look at this too if needed

@shubham-padia
Copy link
Member Author

@timabbott Updated the PR to work for all zoom sizes.

Allow passing image link in the item passed to appendValidatedData.
When passing image link via any of the append* functions, make sure
that create_item_from_text for that pill also adds the image link to
the item created.
This commit does not make any visual change to the current app.
Changes to user_pill.js are necessary to enable user avatars for
pills.
Adding the 20*20 image inside the pill caused a minor increase in
pill height. Making the image 19*19 causes some increase in the height
under different zoom conditions. I'm not sure about the reason behind
this, so this can be counted as a hack.
Fixes zulip#9842.
Enables avatar images in pills wherever user_pill.js is used.
(e.g composebox, user group settings)
Changes to search_pill.js are not made as search pills haven't been
added yet completely and search_pill.js just contains the preparatory
code right now.
No change to compose_pm_pill.js is not required as it uses
`user_pill.create_item_from_text` in its `create` function.
@shubham-padia
Copy link
Member Author

Update: I've made the pill height: 20px change as a separate commit. I've added the following text to the last commit message:

Changes to search_pill.js are not made as search pills haven't been
added yet completely and search_pill.js just contains the preparatory
code right now.
No change to compose_pm_pill.js is not required as it uses
`user_pill.create_item_from_text` in its `create` function.

@timabbott
Copy link
Sponsor Member

Nice, merged, thanks @shubham-padia!

@timabbott timabbott closed this Jul 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants