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

add personal addtional setting section #22144

Closed
wants to merge 1 commit into from
Closed

add personal addtional setting section #22144

wants to merge 1 commit into from

Conversation

dassio
Copy link

@dassio dassio commented Aug 7, 2020

in the developer manual settings section, there is a mentioning of additional default section, but it only appears on the admin part, not on the person part. resulting in apps that are meant for non-admin users can not use the default additional section, this pull request is to add the personal default Additional settings

@dassio
Copy link
Author

dassio commented Aug 25, 2020

are there anyone who can help to review this one ?

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.

Fine by me :)

@ChristophWurst ChristophWurst added this to the Nextcloud 20 milestone Aug 26, 2020
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

There seems to be an issue that the section will still be shown empty if no additional settings are available, could you look into that again?

@juliusknorr
Copy link
Member

Ah seems to be related to the this settings class:

class Additional implements ISettings {

@dassio
Copy link
Author

dassio commented Aug 26, 2020

Ah seems to be related to the this settings class:

class Additional implements ISettings {

@ChristophWurst , can we delete this empty setting, or we have any paticular reason to keep it ?

@dassio
Copy link
Author

dassio commented Aug 26, 2020

@juliushaertl @rullzer @kesselb please help to review
I deleted the empty personal setting view

This was referenced Aug 26, 2020
@dassio
Copy link
Author

dassio commented Sep 1, 2020

can we merge this one now? I just did a rebase on master

* @since 9.1
*/
public function getForm() {
return new TemplateResponse('settings', 'settings/empty');
Copy link
Member

Choose a reason for hiding this comment

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

I'm not entirely sure about the complete removal yet, as it was a fallback for old settings registration iirc, so this would need to be checked if it still works.

Copy link
Author

Choose a reason for hiding this comment

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

you are correct

I tried with this app Ebook Reader 1.4.2, is we remove this, we can't find the setting anymore
I will add it back

Copy link
Author

Choose a reason for hiding this comment

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

I reverted it back and fix the duplicate addtional issue, not sure how to make the empty addtional go away

@rullzer rullzer mentioned this pull request Sep 3, 2020
13 tasks
@kesselb
Copy link
Contributor

kesselb commented Sep 4, 2020

There seems to be an issue that the section will still be shown empty if no additional settings are available, could you look into that again?

Sounds like #22144 @juliushaertl @nickvergessen 🤔

@dassio
Copy link
Author

dassio commented Sep 5, 2020

blank addtional section removed @kesselb

addtional-section-flow-chart

@kesselb
Copy link
Contributor

kesselb commented Sep 5, 2020

blank addtional section removed @kesselb

Sorry. I don't know much about the settings area. It was not my intention to request changes. I just noticed the other pull requested and added a hint. We have to wait for the other reviews to specify the expected behaviour and how those pull requests work together.

@dassio dassio changed the title add personal addtional settign section add personal addtional setting section Sep 7, 2020
@dassio dassio closed this Sep 7, 2020
@dassio
Copy link
Author

dassio commented Sep 7, 2020

solved by #22589

@nickvergessen
Copy link
Member

Sorry @dassio I didn't see your PR before creating mine, and since the one line diff on my is easier I guess that is why it got merged. Thanks for the work anyway!

@nickvergessen
Copy link
Member

this is because there is no an personal addtional section in the settings app, shouldn't we add the section instead of hacking the system ? it will make later comer much eaiser to understand the structure.

While I agree in general, the one liner is much easier to be backported. So let's make it properly (like in the admin section?) with your PR, and only do the minimal impact PR for the backporting

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants