Skip to content

Conversation

Ezio-Sarthak
Copy link
Member

@Ezio-Sarthak Ezio-Sarthak commented Dec 18, 2020

This is a follow-up PR from #849. It aims the following:

  • Popup border and title styling.

  • Popup color styling.

Fixes #684 (for now) :)
The palettes added with this PR might help in styling of footer notifications in #782

@Ezio-Sarthak Ezio-Sarthak force-pushed the re-enhance-popup-styling branch from ec27707 to 5978054 Compare December 18, 2020 16:05
@Ezio-Sarthak Ezio-Sarthak changed the title enhancement: Enclosing the title clearly inside popup. enhancement: Further popup styling improvements. Dec 18, 2020
@Ezio-Sarthak Ezio-Sarthak force-pushed the re-enhance-popup-styling branch from 5978054 to 7558244 Compare December 18, 2020 16:24
Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@Ezio-Sarthak This is a nice self-contained improvement compared to the other PR 👍

There is currently an issue with the popup height, but more importantly I'd really also like to avoid the gaps at the top - perhaps we could just put a linebox around the scrolling content to avoid the difference at the edge of that area and the title. We could also use the top of that linebox to make a slight half-box line above the content to give a small gap at the top between the title and the first line of content.

I also noticed that this looks strange in color-depth=1 at the top.

@neiljp neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Dec 20, 2020
@Ezio-Sarthak Ezio-Sarthak force-pushed the re-enhance-popup-styling branch from 7558244 to c471bb0 Compare December 21, 2020 10:47
@zulipbot zulipbot added the size: M [Automatic label added by zulipbot] label Dec 21, 2020
@neiljp neiljp added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Dec 22, 2020
@zulipbot zulipbot added size: L [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels Dec 24, 2020
@Ezio-Sarthak Ezio-Sarthak force-pushed the re-enhance-popup-styling branch from 546c20a to b56c4be Compare December 24, 2020 09:45
@zulipbot zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Dec 24, 2020
@Ezio-Sarthak Ezio-Sarthak force-pushed the re-enhance-popup-styling branch from b56c4be to 4d8b886 Compare December 24, 2020 09:47
@zulipbot zulipbot added size: L [Automatic label added by zulipbot] and removed size: XL [Automatic label added by zulipbot] labels Dec 24, 2020
@Ezio-Sarthak Ezio-Sarthak requested a review from neiljp December 29, 2020 06:48
@Ezio-Sarthak Ezio-Sarthak force-pushed the re-enhance-popup-styling branch from 4d8b886 to 322ff07 Compare December 29, 2020 08:54
Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@Ezio-Sarthak This looks cleaner now 👍

Layout-wise I think the remaining issue is that the title looks off-center vertically, along with the merging of the title into the background in 1-color mode. Perhaps a half-bar of border color above it?

@neiljp neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Jan 2, 2021
@Ezio-Sarthak Ezio-Sarthak force-pushed the re-enhance-popup-styling branch 2 times, most recently from 2e5bbb1 to 20ea181 Compare January 2, 2021 12:50
@Ezio-Sarthak
Copy link
Member Author

Ezio-Sarthak commented Jan 2, 2021

Thanks for the suggestions!

I've updated the PR. I've corrected the small mistakes I did 😅

@Ezio-Sarthak Ezio-Sarthak force-pushed the re-enhance-popup-styling branch from 20ea181 to 4ad544b Compare January 2, 2021 12:59
@Ezio-Sarthak Ezio-Sarthak requested a review from neiljp January 3, 2021 03:02
Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@Ezio-Sarthak Thanks for addressing my points 👍

While you've made the header higher, the title block looks taller too, ie. a half-line of eg. green below the line of white. Could we move the 'white' half-line down to centre the title text vertically between two white bars? I'm not sure what style to use, maybe a different character with the border style? That may need investigating with respect to 1-color rendering again.

Does this look similar on all themes? I checked 'light' and the title doesn't look quite the same compared to 'default' (zt_light vs zt_dark)

@Ezio-Sarthak Ezio-Sarthak force-pushed the re-enhance-popup-styling branch 2 times, most recently from bc7dccb to 7ba8f0e Compare January 3, 2021 15:05
@Ezio-Sarthak Ezio-Sarthak force-pushed the re-enhance-popup-styling branch from 7ba8f0e to 997cd2c Compare January 3, 2021 17:24
@neiljp neiljp added feedback wanted and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jan 3, 2021
This is a follow-up commit from PR zulip#849.

Visible improvements:
 * Title standouts more in a popup.
 * Popup border blends with the background.
This commit adds:
 * color-per-category for each popup box
 * A sementic option in show_pop_up function call

The categories as of now are help, error, message and stream.

Fixes zulip#684.
@Ezio-Sarthak Ezio-Sarthak force-pushed the re-enhance-popup-styling branch from 997cd2c to ae6ab91 Compare January 6, 2021 15:59
@neiljp
Copy link
Collaborator

neiljp commented Jan 7, 2021

@Ezio-Sarthak Merged with adjusted commit text as e08fbeb and prior commit 🎉

I also slightly adjusted the 1-bit style in the first commit to match that used later (title -> area), and moved the top border style & character from the second to the first commit. If you haven't already I'd recommend giving git range-diff a try to show this kind of change :)

@neiljp neiljp closed this Jan 7, 2021
@neiljp neiljp added this to the Next Release milestone Jan 7, 2021
@Ezio-Sarthak
Copy link
Member Author

git range-diff seems great to distinguish between different commits!

@Ezio-Sarthak Ezio-Sarthak deleted the re-enhance-popup-styling branch March 26, 2021 04:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback wanted size: L [Automatic label added by zulipbot]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance the general popup styling
3 participants