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

typeahead: Make them look like dropdown widget. #30072

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

amanagr
Copy link
Member

@amanagr amanagr commented May 13, 2024

This attempts to change background color and text color of typeaheads to be same as dropdown widgets we have in the app.

active typeahead color was taken from https://terpimost.github.io/compose-decomposed/

Related issue - #25116

This PR is to help us get the colors we want for the typeaheads, then we can do some of the much needed cleanup work in this space.

Here is a list of all typeaheads:

  • Compose box textarea with various triggers - @,#, /, ....
Screenshot 2024-05-13 at 4 01 15 PM Screenshot 2024-05-13 at 4 01 27 PM Screenshot 2024-05-13 at 4 08 54 PM Screenshot 2024-05-13 at 4 09 26 PM Screenshot 2024-05-13 at 4 09 33 PM
  • Compose box topic / DM
Screenshot 2024-05-13 at 4 02 22 PM Screenshot 2024-05-13 at 4 02 39 PM Screenshot 2024-05-13 at 4 09 33 PM Screenshot 2024-05-13 at 4 09 26 PM
  • Message edit box
Screenshot 2024-05-13 at 4 04 23 PM Screenshot 2024-05-13 at 4 08 13 PM Screenshot 2024-05-13 at 4 05 27 PM Screenshot 2024-05-13 at 4 06 51 PM Screenshot 2024-05-13 at 4 06 20 PM Screenshot 2024-05-13 at 4 07 04 PM Screenshot 2024-05-13 at 4 05 55 PM Screenshot 2024-05-13 at 4 07 51 PM

@amanagr amanagr force-pushed the typeahead_style branch 2 times, most recently from c5d03f8 to ab46bc1 Compare May 13, 2024 10:47
@alya
Copy link
Contributor

alya commented May 13, 2024

Is it easy to add a commit here that would make the poll, todo, etc. explanations not be bold, so that they are styled like pronouns? I feel like that would be a clear improvement.

Screenshot 2024-05-13 at 10 31 33@2x

@alya
Copy link
Contributor

alya commented May 13, 2024

Actually, similarly for "(Notify channel)" / "(Notify topic)", and playground languages. It seems like a good approach to take for all the parentheticals in these typeaheads.

If it's non-trivial, we can pull that out into a separate issue or PR.

@alya
Copy link
Contributor

alya commented May 13, 2024

It's helpful to these all alongside each other!

@alya
Copy link
Contributor

alya commented May 13, 2024

The purplish text looks too faded to me, but I'm happy to try it out on CZO and see what other folks think. @terpimost FYI

@alya alya added the chat.zulip.org review Added by maintainers when a PR requires chat.zulip.org testing to proceed. label May 13, 2024
@alya
Copy link
Contributor

alya commented May 13, 2024

I played with the PR and didn't have any other feedback.

@terpimost
Copy link
Collaborator

Lets please have some space between dropdown and the target control.
This should be edge to edge:
image
We should give 2-3px of space:
image

@terpimost
Copy link
Collaborator

If we don't have thing on the left side, why should we reserve any space for it?
image

@alya
Copy link
Contributor

alya commented May 13, 2024

Since @terpimost's feedback is on improvements (not regressions in the current PR), addressing these points isn't a blocker for CZO deployment. @timabbott FYI

@amanagr
Copy link
Member Author

amanagr commented May 14, 2024

Thanks for the feedback! I added 2px gap between the typeahead and the reference.

Screenshot 2024-05-14 at 10 32 01 AM

Below feedback needs to be done in a separate PR because they will be non-localized changes and I would love to do some CSS cleanup before that.

@timabbott
Copy link
Sponsor Member

The space on the left issue noted in #30072 (comment) is the main thing that I kinda want fixed before this goes to production.

@timabbott
Copy link
Sponsor Member

Test-deploying on chat.zulip.org

@timabbott timabbott added the deployed on chat.zulip.org Added by maintainers when a PR is currently being tested on chat.zulip.org. label May 14, 2024
@amanagr
Copy link
Member Author

amanagr commented May 15, 2024

before after
Screenshot 2024-05-15 at 4 33 18 PM Screenshot 2024-05-15 at 5 01 11 PM
before after
Screenshot 2024-05-15 at 4 33 06 PM Screenshot 2024-05-15 at 5 00 19 PM
before after
Screenshot 2024-05-16 at 8 23 24 AM Screenshot 2024-05-15 at 4 30 06 PM
before after
Screenshot 2024-05-16 at 8 24 59 AM Screenshot 2024-05-15 at 4 30 47 PM

Okay updated to remove the extra left padding.

@alya
Copy link
Contributor

alya commented May 15, 2024

Hm, I don't quite follow the last set of before/after screenshots. The idea was just to remove the blank space on the left for the typeaheads that don't have an icon there (slash commands, languages, topics).

@amanagr
Copy link
Member Author

amanagr commented May 16, 2024

Hm, I don't quite follow the last set of before/after screenshots. The idea was just to remove the blank space on the left for the typeaheads that don't have an icon there (slash commands, languages, topics).

I updated the screenshots for last 2 to help do a proper comparison, but I reduced left padding from 20px to 10px for all the typeaheads while making sure typeaheads with user presence circle don't break. We also use the same left padding in our stream selector dropdown in compose box, so I think it will work well.

@amanagr amanagr force-pushed the typeahead_style branch 3 times, most recently from d85c34f to 6b00bc0 Compare May 17, 2024 04:15
This attempts to change background color and text color of typeaheads
to be same as dropdown widgets we have in the app.
This required taking special care of typeaheads with user circle
and making sure long typeaheads wrap correctly.
@timabbott
Copy link
Sponsor Member

Updated deployment of this on chat.zulip.org.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chat.zulip.org review Added by maintainers when a PR requires chat.zulip.org testing to proceed. deployed on chat.zulip.org Added by maintainers when a PR is currently being tested on chat.zulip.org. size: XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants