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

Public page template response #8051

Merged
merged 17 commits into from
Feb 27, 2018
Merged

Public page template response #8051

merged 17 commits into from
Feb 27, 2018

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Jan 25, 2018

This PR is a proof of concept to fix #6553

The header bar should be the same across all public pages of apps, therefore I tried to add a simple abstraction layer to that. We should stop app developers messing to much with custom HTML and therefore it is now possible to set the following parameters for the header:

  • Title (Filename in files_sharing public page)
  • Details ("shared by user" in files_sharing public page)
  • Menu actions (will show the single action with icon or the first with a 3-dots menu for the other actions)

Example usage is shown for files_sharing https://github.com/nextcloud/server/compare/public-template#diff-d6e103c0f5540d507d9c9b6c7ade2151R432

Actions are a simple class containing the icon, label, link and a possible details label, but apps can add their own types by implementing an IMenuAction.

I'm still not sure if that abstraction is to much, but I like having a simple interface to setup those basic things from an app developers perspective. Maybe we need to move out the HTML from the IMenuAction implementations to some files in the template directories. I decided for now to keep the templating code inside the classes, since otherwise we would have to load a template file for each menu entry. We possibly can improve on that later. Ideally there should be some template engine providing proper caching for templates.

Some feedback would be nice @rullzer @MorrisJobke @ChristophWurst @nickvergessen
maybe also from @nextcloud/designers if there are any other default components we should have for public pages.

With one action added:
bildschirmfoto vom 2018-01-25 20-40-09

With multiple actions added:
bildschirmfoto vom 2018-01-25 20-39-27

@codecov
Copy link

codecov bot commented Jan 25, 2018

Codecov Report

Merging #8051 into master will decrease coverage by 0.13%.
The diff coverage is 31.17%.

@@             Coverage Diff              @@
##             master    #8051      +/-   ##
============================================
- Coverage     51.88%   51.75%   -0.14%     
- Complexity    25394    25425      +31     
============================================
  Files          1603     1605       +2     
  Lines         95214    95224      +10     
  Branches       1379     1377       -2     
============================================
- Hits          49403    49282     -121     
- Misses        45811    45942     +131
Impacted Files Coverage Δ Complexity Δ
...iles_sharing/composer/composer/autoload_static.php 0% <ø> (ø) 1 <0> (ø) ⬇️
lib/private/TemplateLayout.php 0% <0%> (ø) 49 <0> (+1) ⬆️
apps/files_sharing/templates/public.php 0% <0%> (ø) 0 <0> (ø) ⬇️
...es_sharing/composer/composer/autoload_classmap.php 0% <0%> (ø) 0 <0> (ø) ⬇️
lib/private/legacy/template.php 29.41% <0%> (-0.59%) 42 <0> (+2)
core/templates/layout.public.php 0% <0%> (ø) 0 <0> (?)
lib/public/AppFramework/Http/TemplateResponse.php 70.83% <0%> (ø) 9 <0> (ø) ⬇️
apps/files_sharing/lib/Template/LinkMenuAction.php 20% <20%> (ø) 2 <2> (?)
...s_sharing/lib/Template/ExternalShareMenuAction.php 31.25% <31.25%> (ø) 2 <2> (?)
lib/private/Template/Base.php 45.45% <33.33%> (-1.72%) 14 <0> (+1)
... and 94 more

@MorrisJobke
Copy link
Member

In general I like this idea 👍 Haven't had a look at the code.

@rullzer
Copy link
Member

rullzer commented Jan 26, 2018

Awesome idea @juliushaertl!
Reminds me that I should continue #6760 as they complement each other nicely :)

@oparoz
Copy link
Member

oparoz commented Jan 29, 2018

Make sure scrolling still works on iOS Safari when having long pages. Last time the structure was changed, performance was unacceptable.

@juliusknorr
Copy link
Member Author

@oparoz Do you have any reference to the last change there, so I can have a look what should be done? Otherwise testing by any iOS user is appreciated. 😉 The only change from the HTML layout perspective is affecting the menu actions in the top right. Everything else like the rendering of the files_sharing pages should stay untouched.

@juliusknorr juliusknorr force-pushed the public-template branch 3 times, most recently from d2eb42c to 687d7a8 Compare February 9, 2018 16:04
@juliusknorr juliusknorr changed the title [PoC] Public page template response Public page template response Feb 9, 2018
@juliusknorr juliusknorr added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Feb 9, 2018
@juliusknorr
Copy link
Member Author

Hm, acceptance tests are still failing. I'll have a look.

@pixelipo
Copy link
Contributor

Is this ready for review or still in Development, @juliushaertl ?

@juliusknorr
Copy link
Member Author

@pixelipo Ah forgot to mention, ready for review. 😉

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Fine by me - only have one little comment.

* @param string $id
* @since 14.0.0
*/
public function setId(string $id) {
Copy link
Member

Choose a reason for hiding this comment

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

Are all those setters really needed? Isn't the stuff in the constructor fine?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm right, there's no real need for those. Removed them.

@rullzer rullzer added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Feb 27, 2018
@rullzer rullzer merged commit 01f420c into master Feb 27, 2018
@rullzer rullzer deleted the public-template branch February 27, 2018 12:33
@skjnldsv
Copy link
Member

Awesome!! 💯
To the documentation now @juliushaertl 🚀 😆

@desaintmartin
Copy link

desaintmartin commented Feb 28, 2018

Hello,
This PR seems to break the public page of a shared folder in the Gallery app. Maybe an update of Gallery is needed?

Here is my log using the latests commits from server.git and gallery.git :

    Message: Call to a member function getHeaderTitle() on null
    File: /srv/www/nextcloud-dev-test.watercooled.org/core/templates/layout.public.php
    Line: 36


Trace

#0 /srv/www/nextcloud-dev-test.watercooled.org/lib/private/Template/Base.php(179): include()
#1 /srv/www/nextcloud-dev-test.watercooled.org/lib/private/Template/Base.php(151): OC\Template\Base->load('/srv/www/nextcl...', Array)
#2 /srv/www/nextcloud-dev-test.watercooled.org/lib/private/legacy/template.php(204): OC\Template\Base->fetchPage(Array)
#3 /srv/www/nextcloud-dev-test.watercooled.org/lib/private/legacy/template.php(235): OC_Template->fetchPage(Array)
#4 /srv/www/nextcloud-dev-test.watercooled.org/lib/public/AppFramework/Http/TemplateResponse.php(157): OC_Template->fetchPage(Array)
#5 /srv/www/nextcloud-dev-test.watercooled.org/lib/private/AppFramework/Http/Dispatcher.php(119): OCP\AppFramework\Http\TemplateResponse->render()
#6 /srv/www/nextcloud-dev-test.watercooled.org/lib/private/AppFramework/App.php(117): OC\AppFramework\Http\Dispatcher->dispatch(Object(OCA\Gallery\Controller\PageController), 'publicIndex')
#7 /srv/www/nextcloud-dev-test.watercooled.org/lib/private/AppFramework/Routing/RouteActionHandler.php(47): OC\AppFramework\App::main('PageController', 'publicIndex', Object(OC\AppFramework\DependencyInjection\DIContainer), Array)
#8 [internal function]: OC\AppFramework\Routing\RouteActionHandler->__invoke(Array)
#9 /srv/www/nextcloud-dev-test.watercooled.org/lib/private/Route/Router.php(297): call_user_func(Object(OC\AppFramework\Routing\RouteActionHandler), Array)
#10 /srv/www/nextcloud-dev-test.watercooled.org/lib/base.php(981): OC\Route\Router->match('/apps/gallery/s...')
#11 /srv/www/nextcloud-dev-test.watercooled.org/index.php(37): OC::handleRequest()
#12 {main}


@rullzer
Copy link
Member

rullzer commented Feb 28, 2018

@juliushaertl and to the gallery it seems ;)

@juliusknorr
Copy link
Member Author

I'll prepare some follow up pull requests for other apps that use public pages.

@juliusknorr
Copy link
Member Author

From a first look int seems that gallery is using 'public' as layout, which was not defined before this PR. Chaning the last argument from this line https://github.com/nextcloud/gallery/blob/2005c66ab59a9dcd66d6794834434750eabea792/lib/Controller/PageController.php#L235 to 'base' should work around the issue until I prepared a proper PR to move to the new public layout.

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 enhancement feature: sharing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add public page template for apps
7 participants