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
Design fixes #89
Design fixes #89
Conversation
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
…yout Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice change!
Can we add a space between the text and the buttons? @jancborchardt |
I also find the before lineheight to narrow, but afterwards it looks like four separate sections. Could there be something in between? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to change my mind a bit, I find the space between paragraph a little bit too big! :)
@nickvergessen actually the space is good and should stay that way for vertical rhythm. However the title should probably be semibold. Will fix the details. |
The space between the rows of the headline is more than between headline and message... I think thats awkward |
I agree with @nickvergessen 's las comment - line height shouldn't be larger than paragraph margin/padding: |
@jancborchardt are you going to do something here until Nextcloud 13? |
I would say we should merge as it is better than currently. Additional fixes can always be done afterwards, but should nt block this PR. |
What’s the current state? Anything for 13? |
how do I test this? any smart way to push a notification on test? |
There is an occ command provided by the admin_notifications app:
|
@MorrisJobke |
As I wrote: this is provided by the admin_notifications app 😉 https://github.com/nextcloud/admin_notifications |
OK, I'll push a commit to this if no one objects |
Found (and fixed) one more bug in this PR - missing bottom margin for notifications that don't have long-message text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks a lot better 👍
I just noticed one bug:
|
Signed-off-by: Marin Treselj <marin.treselj@forlagshuset.no>
Good catch, @MorrisJobke - because of the old CSS solution, notifications are generated upside-down in DOM. I didn't want to mess with JS for now, so I just applied a neat css fix to reorder them ( |
@skjnldsv you requested changes on this PR - is it good now? 😉 |
The issue @MorrisJobke noted was already fixed in master (via JS I assume), so no need for the CSS fix @pixelipo :) Also this is how it looks like rebased on master: I’d say every entry could use a bit more whitespace to the bottom? Cause now it looks a bit like the content is hanging down. |
Ah - I thought that this was rebased since then. Sorry - then it's my fault and the CSS fix can be dropped again. |
Agree with Jan, and the icon have too much top whitespace! :) |
which icon, @skjnldsv ? @MorrisJobke I'll revert the order fix |
@pixelipo the gear icon on jan's example :) |
@skjnldsv gear icon has no whitespace above - space above is (and should be) reserved for notification-heading (close button and timestamp). We could move notification icon there, but it would look weird and we can't move both that and the title, because there's not enough space. |
Signed-off-by: Marin Treselj <marin.treselj@forlagshuset.no>
Signed-off-by: Marin Treselj <marin.treselj@forlagshuset.no>
The icon doesn't have too much top whitespace, it's aligned with the text. :) as said above:
|
Signed-off-by: Marin Treselj <marin.treselj@forlagshuset.no>
Ok, done - let's merge this bad boy, @jancborchardt @skjnldsv @MorrisJobke If there are any additional small fixes needed, I think we should make them later, because this should go into NC13 as soon as possible. |
css/styles.css
Outdated
|
||
.notification-subject a { | ||
display: inline-block; | ||
display: flex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation is off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and works 👍 Looks good
Signed-off-by: Marin Treselj <marin.treselj@forlagshuset.no>
I fixed some minor issues in nextcloud/server#7572 |
🎉 |
@@ -18,7 +9,8 @@ | |||
min-height: 100px; | |||
max-height: 260px; | |||
border-radius: 0 0 3px 3px; | |||
border: 1px solid rgb(238, 238, 238); | |||
overflow: hidden; | |||
overflow-y: auto; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving this here from the .notification-wrapper
hides the drop shadow as well as the little caret in Safari. Let me fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix is in #106
Before & after:
Please review @nickvergessen @nextcloud/designers