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

make a button out of 'What's new' so it's obvious it can be clicked on #12757

Merged
merged 2 commits into from
Dec 5, 2018

Conversation

violoncelloCH
Copy link
Member

for #12694

Signed-off-by: Jonas Sulzer jonas@violoncello.ch

@violoncelloCH violoncelloCH added 3. to review Waiting for reviews papercut Annoying recurring issue with possibly simple fix. feature: settings labels Nov 30, 2018
@violoncelloCH violoncelloCH added this to the Nextcloud 15 milestone Nov 30, 2018
@ChristophWurst
Copy link
Member

Conflicting files

apps/updatenotification/js/updatenotification.js
apps/updatenotification/js/updatenotification.js.map

FYI 😉

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Makes sense!

@violoncelloCH
Copy link
Member Author

Conflicting files

apps/updatenotification/js/updatenotification.js
apps/updatenotification/js/updatenotification.js.map

FYI wink

@ChristophWurst Does this mean I have to rebase my PR?

@skjnldsv
Copy link
Member

skjnldsv commented Dec 3, 2018

@violoncelloCH rebase, ignore the changes on those two files, compile again, git add changes and finish rebase :)

@jancborchardt
Copy link
Member

Btw @violoncelloCH for design fixes it’s always good to add a quick screenshot so others can verify it looks like you want it to. 🙂

for #12694

Signed-off-by: Jonas Sulzer <jonas@violoncello.ch>
@violoncelloCH
Copy link
Member Author

violoncelloCH commented Dec 3, 2018

@skjnldsv thank you -- rebase done
@jancborchardt yes, I will do this next time. Problem is for a correct screenshot I would have needed to fake an update response from the server again, what would have taken me some time... therefore here a screenshot of the button but with an empty popover:
image

@kesselb
Copy link
Contributor

kesselb commented Dec 3, 2018

#12677 (comment) when you need a fake updater response ;-)

@rullzer rullzer modified the milestones: Nextcloud 15, Nextcloud 16 Dec 4, 2018
@jancborchardt
Copy link
Member

@violoncelloCH no problem! :) It looks good, but could you also add class="primary" to the "Open updater" button? Because now the 2 same-looking buttons next to each other look a bit strange, and we should signify which is the important one.

I'd even say we should put the "What's new" right next to the other button.

What do you think?

@violoncelloCH
Copy link
Member Author

violoncelloCH commented Dec 4, 2018

It looks good, but could you also add class="primary" to the "Open updater" button? Because now the 2 same-looking buttons next to each other look a bit strange, and we should signify which is the important one.

of course, will do this

I'd even say we should put the "What's new" right next to the other button.

so we've got "Open updater" or "Download now" and "What's new" all in one row? should I change it like that? @jancborchardt

@violoncelloCH
Copy link
Member Author

hmm, the "open updater" and "download now" buttons are inside a <p> which cannot include <div>s, but "what's new" needs <div>s...
I now changed the <p> to a <div> as well what should work. Hope this doesn't break something else

Signed-off-by: Jonas Sulzer <jonas@violoncello.ch>
@jancborchardt
Copy link
Member

Yes, I think having all buttons in one row looks better. With the most important action being on the left and with class="primary". Thanks a lot @violoncelloCH! :)

@skjnldsv
Copy link
Member

skjnldsv commented Dec 5, 2018

Test restarted. Lots of unrelated timeouts

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Dec 5, 2018
@MorrisJobke MorrisJobke merged commit 2c30664 into master Dec 5, 2018
@MorrisJobke MorrisJobke deleted the whatsNew-to-button branch December 5, 2018 11:34
@MorrisJobke
Copy link
Member

/backport to stable15

@backportbot-nextcloud
Copy link

The backport to stable15 failed. Please do this backport manually.

@MorrisJobke MorrisJobke mentioned this pull request Dec 5, 2018
12 tasks
skjnldsv added a commit that referenced this pull request Dec 5, 2018
…#12757

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv
Copy link
Member

skjnldsv commented Dec 5, 2018

Backport in #12848

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish feature: settings papercut Annoying recurring issue with possibly simple fix.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants