Fix DOM problems and styling

[LordSimal]

Hello guys,

since I updated my NC Server to 17 and this App was activated by default, I recognised, that the styling can be improved.

What have I done?

Basically I have refactored the DOM (since there were some unclosed HTML tags), adjusted the styling according to the new DOM and refactored the JS.

Main factor on the CSS side was, that I introduced a CSS Grid (see at the bottom of style.css) based on the bootstrap row-col system with 12 columns.
But I only implemented the responsive classes, that I thought were necessary.

Also I had to adjust the appinfo/info.xml so this app has "official" support for NC 17.
Otherwise I couldn't install the app.

The result

https://i.pfiff.me/refactored-serverinfo-app.mp4

This would fix issues:

Fix #158, Fix #155

[nickvergessen]

@nextcloud/designers anyone willing to install this and take a look and review it?

[skjnldsv]

@LordSimal aside from fixing the two issues you mentionned, any other changes?
Could you also add screenshots of the area you changed (if really different)

Thanks for your help and sorry we missed your awesome pr! :)

[LordSimal]

Hello @skjnldsv,

ok, didn't see that branch therefore reverted the change in the appinfo.info.xml
I also fixed the tab indentation. I would only suggest to insert a .editorconfig file which would define the recommended indentation.

The main "change" here is the layout of the different elements.

I didn't add any functional code, just refactored the DOM of the settings-admin.php and with that adjusted some classes and styling.

My main problem here was, that the elements weren't using the space that was given on the desktop variant very efficiently. Therefore I introduced a flex based Grid in the bottom of the style.css and implemented this grid in the refactored DOM from above.

Also there were DOM errors present like missing

which have been resolved with that.

Old

https://i.pfiff.me/serverinfo-app-old.mp4

New

https://i.pfiff.me/refactored-serverinfo-app.mp4

[nickvergessen]

Our editorconfig is in the server repo:
https://github.com/nextcloud/server/blob/master/.editorconfig

It should work when you check it out inside server repo which most people do for local development anyway

[skjnldsv]

also fixed the tab indentation

This is still not fixed, we indent with a tab character here, not 4 spaces :)

[jancborchardt]

@LordSimal good stuff! :) Could you use the class="section" to group things instead of these boxes? That would align it a lot with the rest of Nextcloud.

The sections would be:

• Server name
• Hard drives (the individual hard drives can still be containers)
• Network (the individual network connections can still be containers)
• Active users
• Shares
• PHP
• Database
• External monitoring

What do you think?

[LordSimal]

As desired by @jancborchardt I added sections to the DOM.

I also moved all the headings outside of the infoboxes and added 2 new icons (profile silhouette and a folder icon)

See

and

Also, as mentioned above, I added the given .editorconfig from the server repository to this repo and reformated the whole project with PHPStorm.
This seems to have produced some conflicts.

[LordSimal]

Sorry, im dumb.

I forgot to rebase my fork to the current master.
I am currently resolving those conflicts, please stand by :P

[LordSimal]

well reading carefully would help as well. Sorry for the many comments, kind of a messy pull request now^^

I understood @jancborchardt message to insert <section> DOM elements, but actually he wanted just a wrapper with the class section.

Will try to adjust that now.

[LordSimal]

@jancborchardt you really want each described section with that much padding on all sides?
I kinda don't like that
see

[jancborchardt]

you really want each described section with that much padding on all sides? I kinda don't like that

Check how we do it on the other settings pages. :) It’s generally always in the style of:

<div class="section">
   <h2>Section title</h2>
   <p>Bla blabla section content</p>
</div>

<div class="section">
   <h2>Another section</h2>
   <p>Section content etc</p>
</div>

Please use the same structure here so it looks uniform. :)

[LordSimal]

@jancborchardt
I now added the "section" class to all "sections" similar to all other settings pages and adjusted the styling so everything is uniform with all the rest of the settings pages.

[jancborchardt]

@LordSimal looks much better! I’d only also put the "External monitoring program" at the very bottom into its own section.

Otherwise 👍 on the enhancement

[LordSimal]

@jancborchardt as desired I put the "External monitoring program" into its own section.
Also I found some problems with the canvases while resizing the windows. Fixed that as well.

[kesselb]

Nice 👍

Would you mind to revert the changes unrelated to this pr? For example all files in l10n because they will be updated by a bot. Also there are many whitespace changes.

[LordSimal]

@kesselb
yeah, sorry for all the unnecessary commits, I'm used to reformat the code im working with in my own coding style. This is basically my first open-source project I contributed to with a PR.
As desired I copied the current version of the master over my current fork to restore all files as they were but leaving all the files I have edited.

[kesselb]

Thanks 👍

yeah, sorry for all the unnecessary commits, I'm used to reformat the code im working with in my own coding style.

No problem. We can squash them later into one commit.

[kesselb]

Thanks for this great contribution 👍 Some smaller nitpicks. Feel free to ignore nice to have comments.

[kesselb]

To any other reviewer ;)

I'm unable to apply https://patch-diff.githubusercontent.com/raw/nextcloud/serverinfo/pull/164.patch but https://patch-diff.githubusercontent.com/raw/nextcloud/serverinfo/pull/164.diff works. Only the user.svg and folder.svg ended up in the wrong directory (but this seems to be a phpstrom hiccup).

[LordSimal]

@juliushaertl as desired I rebased my fork

[kesselb]

Hey @juliushaertl, I would like to improve some more things. Could you review this PR a second time and merge it? Otherwise we probably do another rebase round ;)

[LordSimal]

I also found some more improvements to be made, but I have also learned, that I should create a PR for each little feature instead of doing it all together ^^
Therefore I agree that we should first of all merge the current state and improve on that.

[nickvergessen]

That rebase seems to not have worked out.

Are all the important commits from you? Or did someone else also push stuff here. Then I can rebase this and scrap all the wrong commits.

[LordSimal]

I really don't know what I should do now.
But actually I really don't care any more... Someone can take over this if they want.
Just tell me what I have to do.

[kesselb]

I pushed some commits to his branch.

[kesselb]

https://github.com/nextcloud/serverinfo/pull/164/files#diff-2f86cb96a0233e7cb3b6f03ad573be0bR38 seems to be a leftover from the merge.

I tried to rebase the branch locally but to many conflict. I'm totally lost which changes are relevant and which not.

[nickvergessen]

Okay so we all 3 are at the same point 💃

So let me try a rebase with the pick list from above and see if it still looks good and shiny

[nickvergessen]

I don't know, the history is duplicated in #185 but it seems to work.
I fixed the dark theming option, have a quick check and then we merge #185

[nickvergessen]

Thanks a lot again @LordSimal for stepping up and polishing the UI so hard.
I merged it now via #185

You can now reset your own master to upstream/master

I also invited you to the organisation, once you accepted the invite, you can create branches and push them to our repo, that allows easier collaboration in the future :)

I hope you stay on board and sorry that it took so long.