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

input-pill: Refactor pills to eliminate fixed positioning of elements. #9941

Closed
wants to merge 1 commit into from

Conversation

synicalsyntax
Copy link
Member

screenshot at jul 12 09-16-49
screenshot at jul 12 09-17-01

Removes fixed positioning from pills so that their alignment and positioning won't break at different zoom sizes.

@timabbott
Copy link
Sponsor Member

It looks like the "X" got considerably smaller?

Otherwise lgtm, though I think it might be worth adding to this PR the change that @armaanahluwalia suggested of increasing the whitespace between two pills by a few pixels (not sure how many is best).

@armaanahluwalia
Copy link
Contributor

Awesome work here with the CSS cleanup. Basically i think the second pill should be positioned where the blinking cursor is once the first pill is in place.

@synicalsyntax
Copy link
Member Author

screenshot at jul 12 14-45-54

@armaanahluwalia
Copy link
Contributor

@synicalsyntax We're talking about number 2 in the picture ( Essentially adding a few pixels to the right margin of each pill. My guess is you could gauge how much space to add by making a second pill begin where the cursor blinks to the right of the first pill ).

Tim is also saying he feels the 'x' icon somehow is smaller than the current version. Maybe you could confirm.

@synicalsyntax
Copy link
Member Author

screenshot at jul 12 14-58-18
screenshot at jul 12 14-58-26

Is this better?

@armaanahluwalia
Copy link
Contributor

@synicalsyntax How come one x looks bigger than the other?

@synicalsyntax
Copy link
Member Author

synicalsyntax commented Jul 12, 2018

@armaanahluwalia I was comparing the original size with the new icon size. Here's the current one on this PR:

screenshot at jul 12 15-15-47

This is the size on chat.zulip.org:
screenshot at jul 12 15-16-37

@armaanahluwalia
Copy link
Contributor

armaanahluwalia commented Jul 12, 2018

Looks good to me. Maybe 1px or 2px more on the margin-right of the pill thats all.

@synicalsyntax
Copy link
Member Author

screenshot at jul 12 16-25-24

Personally, I think we have too much margin now; pills aren't supposed to be cramped, but they're supposed to be compact in their pill containers as well.

@armaanahluwalia
Copy link
Contributor

You're right. IMO the container in night mode is not necessary in this case. We could just add background transparent to it so it disappears just like its not visible in normal mode.

@synicalsyntax
Copy link
Member Author

synicalsyntax commented Jul 13, 2018

Going to wait for feedback from @timabbott before I make any more changes. Current status:

@timabbott
Copy link
Sponsor Member

I'm happy with the between-pills spacing there. It looks like we lost a bit of padding around the name in the pills which makes them feel a bit cramped; can you add a bit there to better match what's currently on chat.zulip.org?

@synicalsyntax
Copy link
Member Author

screenshot at jul 13 10-06-40

Is this better?

<div class='exit'>
&times;
<span class="pill-value">{{ display_value }}</span>
<div class="exit" aria-label="{{t Remove pill }}">
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

The aria-label here is invalid -- the string should be quoted. But I also don't think we really need the aria-label here; this isn't a widget you would want to interact with as a blind user (instead, you just hit backspace to get rid of it, I think?).

@timabbott
Copy link
Sponsor Member

I removed the broken aria-label and merged this, thanks @synicalsyntax! Definitely looks nicer.

@timabbott timabbott closed this Jul 23, 2018
@synicalsyntax synicalsyntax deleted the pills branch July 23, 2018 19:45
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

4 participants