Narrow symbols #1428
Narrow symbols #1428
Conversation
There was a problem hiding this comment.
@supascooopa Thanks for pushing this into a PR 👍
I looked at the tests - we have a flakey (unreliable) test, and restarting just one made them complete.
The symbols look good to me, though the star doesn't look as central vertically compared to the others, so not as good perhaps. One aspect of that we can explore is styling the symbols, maybe just bold or something similar. That is what led to me pointing out the prefix_markup, since it's easier to style the prefix separately from the text that way.
Do you plan to add the symbols to the narrows as viewed in the centre title area too?
neiljp
added
the
PR awaiting update
I'm relived to find out that the test was not working properly and not something I did, I was being real careful to have my ducks in a row before pushing to remote this time. I will give I was actually planning to add the prefix symbols in the center title area but I wasn't sure to do it in this PR or next one. To clarify, you mean the area right below 'Messages' title right? |
|
@supascooopa If you're happy managing the prefix symbols in this PR then that's fine, as separate later commits to what you have already 👍 |
zulipbot
added
size: S
|
@neiljp I've adjusted the prefix markers according to your suggestions. I wasn't sure how to implement the styling, add a brand new one to the styles or use an existing one so I just went with the latter. |
|
@supascooopa Thanks for the update 👍 Structurally the code looks simpler using the prefix markup. I'm not entirely sure the bold makes too much difference for me and is sufficient, but with the common style name, we can easily adjust that at a later point. Similarly the star marker looks 'high', but we can adjust later if finding a suitable alternative. To support getting this into a mergeable state, before continuing, I'd suggest combining your most recent commits separately into the earlier two, since they are fixes to each respectively. Then once you have two commits, getting the commit titles according to our standards. If you want to keep the substance of the commits from your previous iteration, then depending upon your comfort level with git, you might find it useful in future to add (or move) the fixing (fixup) commits right after each one you wish to change. That makes it clear if a new commit relates to an update to a specific earlier commit and makes it very simple to combine (fixup or squash) those with the earlier commit later. However, within Zulip it's also totally accepted to push to your branch with a completely fixed/updated set of commits, doing a force-push if necessary. |
supascooopa
force-pushed
the
narrow_symbols
branch
from
3a4998b
to
2d63466
Compare
|
@neiljp Thank you for the suggestion regarding git I will keep it in mind next time. |
|
@supascooopa This looks fine to move ahead with 👍 The first commit adds multiple symbols, not just the all-messages symbol, but we can deal with that detail later or on merging. Are you working on the symbols for the central narrow titles now? |
|
@neiljp Oh yes! I think it would be better to check the changes before writing the message next time. I want to work on adding the symbols to the central narrow titles. Should I work on this draft or open a different one? |
The 'Mentioned messages' and 'Starred messages' symbols are set to ASCII characters, similar to those in the Zulip web app. The selected 'All messages' symbol is the closest match found after some testing, and appears to be available fairly widely. An appropriate descriptive comment is added, as with other non-ASCII symbols, for later reference.
This commit adds prefix symbols to the main buttons in the top left corner of the UI. This aims to make the main buttons stand out more and approximate the designs used in the web app. This change also leads to a cleaner design, since these top buttons are now indented and aligned similarly to the panels beneath them, eg. the list of streams. The symbol used as a prefix in headers of direct messages is reused for the 'Direct messages' button, with other buttons using symbols added in the previous commit. The left part of the UI is increased in width to accommodate the new additions. Note that the "title" style is used to make the icons bolder, though this should be decoupled in future.
neiljp
force-pushed
the
narrow_symbols
branch
from
2d63466
to
a557745
Compare
neiljp
added
PR ready to be merged
|
@supascooopa I just tidied up the commit messages and pushed them back here - that way we can get this merged and in wider use. Please review the differences I made in the commit messages. I also slightly adjusted your additions in the first commit, to ensure the existing and new markers were grouped more cleanly. You can fetch this PR with Note that you could have developed beyond this branch in a 2nd local branch ("on top of" this one), as long as you were willing to keep the 2nd one consistent with any changes to this - that's where you can rebase locally, and handle any conflicts. That can work well if you don't expect the first branch to change too much, while allowing you to continue working beyond that and being ready to push another branch/PR. |
|
Thank you very much @neiljp. I will keep your advice in mind for next time. |
What does this PR do, and why?
Adding symbols to narrow buttons on the top left of the side UI. This is to improve UI visually.
External discussion & connections
topicHow did you test this?
Self-review checklist for each commit
Visual changes
Before:
After:
