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.

Sign up for GitHub

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

Jump to bottom

Trashbin: Delete => Delete permanently #14721

Merged
merged 1 commit into from
Mar 18, 2019
Merged

Trashbin: Delete => Delete permanently #14721

merged 1 commit into from
Mar 18, 2019

Conversation

rullzer
Copy link
Member

@rullzer rullzer commented Mar 16, 2019

For #14639

However it seems the gridview has a limited size for the menu...

delperm_grid

And the name overlays the restore icon

@rullzer
Trashbin: Delete => Delete permanently
9dd7bc1 
For #14639

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer rullzer added 3. to review Waiting for reviews papercut Annoying recurring issue with possibly simple fix. labels Mar 16, 2019
@rullzer rullzer added this to the Nextcloud 16 milestone Mar 16, 2019
@jancborchardt jancborchardt mentioned this pull request Mar 16, 2019
Closed
62 tasks
@faily-bot
Copy link

faily-bot bot commented Mar 16, 2019

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 17089: failure

TESTS=ui-regression

@skjnldsv
Copy link
Member

And the name overlays the restore icon

You need #14695 and then add a css exception for the grid view mode to show the menu entry and hide the outside ones :)
@jancborchardt maybe we could create a global css rule to hide every action outside the menu in grid?
Instead of specific ones 🤔

@jancborchardt
Copy link
Member

However it seems the gridview has a limited size for the menu

Yes, this is intentionally set to the width of the grid as otherwise it looks off. We could make it wider, but it should be centered below the element.

@skjnldsv yep, good point about the standard rule! Needs to happen in the JS, does it?

jancborchardt
jancborchardt approved these changes Mar 17, 2019
View reviewed changes
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

👍

@rullzer
Copy link
Member Author

rullzer commented Mar 17, 2019

All fine by me. But indeed let's fix that all separately

@skjnldsv
Copy link
Member

Needs to happen in the JS, does it?

No, css only :)

@jancborchardt
Copy link
Member

So, second thumbs up on this as first step? ;)

ChristophWurst
ChristophWurst approved these changes Mar 18, 2019
View reviewed changes
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.

👍

@rullzer rullzer merged commit c416875 into master Mar 18, 2019
@rullzer rullzer deleted the fix/14639/trash_delete_perm branch March 18, 2019 09:10
@MorrisJobke MorrisJobke mentioned this pull request Mar 20, 2019
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@jancborchardt jancborchardt jancborchardt approved these changes

@MorrisJobke MorrisJobke Awaiting requested review from MorrisJobke

@skjnldsv skjnldsv Awaiting requested review from skjnldsv skjnldsv is a code owner

Assignees
No one assigned
Labels
3. to review Waiting for reviews papercut Annoying recurring issue with possibly simple fix.
Projects
None yet
Milestone
Nextcloud 16
Development

Successfully merging this pull request may close these issues.
4 participants
@rullzer @skjnldsv @jancborchardt @ChristophWurst