Skip to content

App menu compatibiliy #336

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
Merged
juliusknorr merged 14 commits into from
Nov 27, 2017
Merged

App menu compatibiliy #336

juliusknorr merged 14 commits into from
Nov 27, 2017

Conversation

@juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Oct 26, 2017 •
edited
Loading

This PR fixes the menu für NC13. I also reworked some of the ListController logic to make it more performant.

  • Check for NC 12 compatibility
  • Fix closing the popover menu

fixes #315

@juliusknorr juliusknorr added this to the 0.3.0 milestone Oct 26, 2017
@codecov
Copy link

codecov bot commented Oct 26, 2017 •
edited
Loading

Codecov Report

Merging #336 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #336   +/-   ##
=======================================
  Coverage   78.22%   78.22%           
=======================================
  Files          38       38           
  Lines        1194     1194           
=======================================
  Hits          934      934           
  Misses        260      260

@juliusknorr
Copy link
Member Author

juliusknorr commented Nov 7, 2017

@skjnldsv Any hints on how backward compatibility to stable12/11 should be handled with the new flex app sidebar?

@skjnldsv
Copy link
Member

skjnldsv commented Nov 7, 2017

@juliushaertl Depends on what types of items you got. :)
I'll go for a patch per apps to be removed when lowest version supported is >=13

rpadovani
rpadovani reviewed Nov 8, 2017
View reviewed changes
js/controller/ListController.js
var filter = {};
filter[$scope.status.filter] = true;
$scope.boardservice.sorted = $filter('cardFilter')($scope.boardservice.sorted, filter);
} else if ($scope.status.filter === 'shared') {
Copy link
Contributor

@rpadovani rpadovani Nov 8, 2017

Choose a reason for hiding this comment

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

Isn't better to use enums instead of string to indicate the filter state? :)

Maybe it can go to a separated issue

Copy link
Member Author

@juliusknorr juliusknorr Nov 8, 2017

Choose a reason for hiding this comment

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

Hmm, the filter state is set as a state param where we have it as a string anyway. So until we don't use it more often in the code, i guess that is fine for now. But if you want to improve here, feel free to do so. 😉

@skjnldsv
Copy link
Member

skjnldsv commented Nov 8, 2017

@juliushaertl Please don't hesitate to ask for help! :)

@juliusknorr
Copy link
Member Author

juliusknorr commented Nov 8, 2017

@skjnldsv Thanks. Actually it already works quite nice with those few changes: fa7d9e9#diff-be704e29a41bc5b36d0f4761c521b82d

Right now only works with NC12, since I plan to drop NC11 support once 13 is out.

@skjnldsv
Copy link
Member

skjnldsv commented Nov 8, 2017

Yeah, it's juste a matter of transition! :)
It should not require big patches, simple ones like you did are what's cool about this.

Once 13 will be the lowest supported, it should be fine ;)

@juliusknorr
Copy link
Member Author

juliusknorr commented Nov 8, 2017

Please review @nextcloud/deck

@skjnldsv skjnldsv self-assigned this Nov 17, 2017
pixelipo
pixelipo reviewed Nov 17, 2017
View reviewed changes
css/style.scss Outdated
Copy link
Contributor

@pixelipo pixelipo Nov 17, 2017

Choose a reason for hiding this comment

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

shouldn't import go to the top?

skjnldsv
skjnldsv previously requested changes Nov 17, 2017
View reviewed changes
css/legacy.scss Outdated
Copy link
Member

@skjnldsv skjnldsv Nov 17, 2017

Choose a reason for hiding this comment

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

Why do you add the variables.scss content here?
The scss compilator automatically add this to any file you load with add_style :)

Copy link
Member Author

@juliusknorr juliusknorr Nov 19, 2017

Choose a reason for hiding this comment

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

Not for stable11, but we can drop this since we will only support version 12 or later in the next release.

Copy link
Member

@skjnldsv skjnldsv Nov 19, 2017

Choose a reason for hiding this comment

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

Yes, I asked because the info.xml says min version to 12. :)

@juliusknorr
Frontend: Update app menu to fit new NC13 flex layout
29c6b3d 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Navigation: make editing work again
8dd3b4a 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Refactor boardlist loading process
f2b4f05 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Performance optimizations in list template
7f1ea3b 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Fix board updating reset
812eedb 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Fix menu
38febfa 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Move variables to legacy scss
2cbeea0 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Add fix for nc<13 app menu
dcc1214 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
CSS: Move icons to separate file
e14af53 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
CSS: restore variables that are not part of the server
4e1fbcb 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Fix popovermenus on 13
a7a960b 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Fix popover hiding
9573e66 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Fix overlooked rebasing error
3e68e2d 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Remove legacy css build and move scss imports to the top
4037a03 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr merged commit 175f47b into master Nov 27, 2017
@juliusknorr juliusknorr deleted the app-menu-13 branch November 27, 2017 20:14
@juliusknorr
Copy link
Member Author

juliusknorr commented Nov 27, 2017

Merging so we can move on. 😉

@skjnldsv
Copy link
Member

skjnldsv commented Nov 28, 2017

Well done!! 👌😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pixelipo pixelipo pixelipo left review comments

@skjnldsv skjnldsv skjnldsv left review comments

@jancborchardt jancborchardt Awaiting requested review from jancborchardt

@rpadovani rpadovani Awaiting requested review from rpadovani

@artemanufrij artemanufrij Awaiting requested review from artemanufrij

Assignees

@skjnldsv skjnldsv

Labels

3. to review bug frontend

Projects

None yet

Milestone

0.3.0

Development

Successfully merging this pull request may close these issues.

App navigation is broken on NC 13

5 participants

@juliusknorr @skjnldsv @rpadovani @pixelipo