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

add undo for calendar deletion #148

Closed
wants to merge 1 commit into from
Closed

add undo for calendar deletion #148

wants to merge 1 commit into from

Conversation

georgehrke
Copy link
Member

implements / fixes #12

please review @nextcloud/calendar @jancborchardt

@georgehrke georgehrke added the 3. to review Waiting for reviews label Oct 21, 2016
@mention-bot
Copy link

@georgehrke, thanks for your PR! By analyzing the history of the files in this pull request, we identified @tcitworld, @raghunayyar and @tomneedham to be potential reviewers.

georgehrke
georgehrke commented Oct 21, 2016
View reviewed changes
css/app/calendarlist.css
@@ -212,6 +212,16 @@
box-sizing: border-box;
}

#app-navigation .app-navigation-list-item {
max-height: 1000px;
Copy link
Member Author

Choose a reason for hiding this comment

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

@jancborchardt Is that a very ugly hack or is that fine?

Copy link
Member

@eppfel eppfel Oct 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Just curios: What does it do? Can't spot the difference.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is needed for the transition. You can't define a transition from height:0 to height:auto

Copy link
Member

Choose a reason for hiding this comment

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

@georgehrke the question really is, why is all this general styling which could be useful for other apps in the Calendar and not in core/css/apps.css? ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Because I need those styles now and not with Nextcloud 11 ;)
But yes, eventually we should unify them in the server and add some developer instructions.

@georgehrke georgehrke force-pushed the undo_calendar_deletion branch from 6c6f2dc to fa1368f Compare October 21, 2016 18:30
@coveralls
Copy link

coveralls commented Oct 21, 2016 •
edited
Loading

Coverage Status

Coverage decreased (-0.3%) to 42.627% when pulling fa1368f on undo_calendar_deletion into 03716b8 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 42.627% when pulling fa1368f on undo_calendar_deletion into 03716b8 on master.

eppfel
eppfel reviewed Oct 21, 2016
View reviewed changes
tests/js/unit/controllers/calendarlistcontrollerSpec.js
@@ -73,7 +73,7 @@ describe('CalendarListController', function() {
expect($scope.newCalendarColorVal).toBe('');
});

it ('should delete the selected calendar', function () {
/*it ('should delete the selected calendar', function () {
Copy link
Member

@eppfel eppfel Oct 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Commented Code?

Copy link
Member Author

Choose a reason for hiding this comment

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

The unit tests in this file are really bad unit tests 🙈
I didn't fix the test, because this entire test suite needs to be refactored anyway

@tcitworld
Copy link
Member

Maybe we can add event deletion in the same PR ?

@georgehrke
Copy link
Member Author

Maybe we can add event deletion in the same PR ?

It's a different feature, so I'd like to implement this in a dedicated PR, but definitely in time for 1.5 ;)

@georgehrke georgehrke force-pushed the undo_calendar_deletion branch from fa1368f to f826cf0 Compare October 22, 2016 08:35
@georgehrke
Copy link
Member Author

added a unit test to CalendarListItem test suite

@georgehrke
Copy link
Member Author

One test in timezoneservice will fail, but that's unrelated and will be fixed with #149

@georgehrke georgehrke mentioned this pull request Oct 22, 2016
Closed
@tcitworld
Copy link
Member

Code looks good. Will test asap.

tcitworld
tcitworld reviewed Oct 23, 2016
View reviewed changes
js/app/models/calendarListItemModel.js Outdated
}
}, 7500);

const elm = OC.Notification.showTemporary(t('calendar', 'Undo deletion'));
Copy link
Member

Choose a reason for hiding this comment

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

Same remark than for events : replace with "Calendar has been deleted. Undo ?"

Copy link
Member

Choose a reason for hiding this comment

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

Yup. But with the actual calendar name:

Calendarname has been deleted. Undo?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe even

Calendar calendarname has been deleted

@georgehrke georgehrke force-pushed the undo_calendar_deletion branch 3 times, most recently from f5a558e to 408a599 Compare November 8, 2016 13:04
@georgehrke
Copy link
Member Author

i changed the string and rebased, please check again :)

@georgehrke georgehrke added 1. to develop Accepted and waiting to be taken care of and removed 3. to review Waiting for reviews labels Dec 5, 2016
@georgehrke georgehrke mentioned this pull request Feb 13, 2017
Closed
@tcitworld tcitworld force-pushed the undo_calendar_deletion branch from 408a599 to c349b60 Compare March 23, 2017 12:51
@tcitworld
Copy link
Member

Rebased.

@adrienfr44 adrienfr44 mentioned this pull request Mar 27, 2017
Closed
@georgehrke
Copy link
Member Author

This approach is at a dead end thats only gonna end up in a huge chain of ugly hacks.
The proper solution is gonna be nextcloud/server#1662

@georgehrke georgehrke closed this Feb 27, 2018
@georgehrke georgehrke deleted the undo_calendar_deletion branch February 27, 2018 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tcitworld tcitworld tcitworld left review comments

@eppfel eppfel eppfel left review comments

@jancborchardt jancborchardt jancborchardt left review comments

Assignees
No one assigned
Labels
1. to develop Accepted and waiting to be taken care of
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

UNDO calendar deletion
6 participants
@georgehrke @mention-bot @coveralls @tcitworld @jancborchardt @eppfel