Skip to content

adding some documentation hints and some smaller cleanup #4478

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
nickvergessen merged 6 commits into from
Apr 26, 2017
Merged

adding some documentation hints and some smaller cleanup #4478

nickvergessen merged 6 commits into from
Apr 26, 2017

Conversation

@karlitschek
Copy link
Member

@karlitschek karlitschek commented Apr 24, 2017

@mention-bot
Copy link

mention-bot commented Apr 24, 2017

@karlitschek, thanks for your PR! By analyzing the history of the files in this pull request, we identified @schiessle, @nickvergessen and @SergioBertolinSG to be potential reviewers.

@rullzer rullzer added the 3. to review Waiting for reviews label Apr 24, 2017
@rullzer
Copy link
Member

rullzer commented Apr 24, 2017

@karlitschek you need to sign your commits 😉

schiessle
schiessle reviewed Apr 24, 2017
View reviewed changes
apps/federation/templates/settings-admin.php Outdated
<button id="ocFederationSubmit" class="hidden"><?php p($l->t('Add')); ?></button>
<span class="msg"></span>
</p>
<h3><?php if(count($_['trustedServers'])>0) p($l->t('Trusted servers')); ?></h3>
Copy link
Member

@schiessle schiessle Apr 24, 2017

Choose a reason for hiding this comment

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

I would always show the headline. Even if there is only the "add trusted servers" button, it is still the "Trusted servers" section. Otherwise it looks also strange if you start to add trusted servers, because while you add them they will be listed without the headline and only after reload the headline will appear.

@codecov
Copy link

codecov bot commented Apr 25, 2017 •
edited
Loading

Codecov Report

Merging #4478 into master will decrease coverage by 25.29%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #4478      +/-   ##
============================================
- Coverage      53.9%   28.61%   -25.3%     
+ Complexity    21701    21700       -1     
============================================
  Files          1272     1272              
  Lines         75815    75215     -600     
============================================
- Hits          40870    21523   -19347     
- Misses        34945    53692   +18747
Impacted Files Coverage Δ Complexity Δ
settings/templates/admin/encryption.php 0% <ø> (ø) 0 <0> (ø) ⬇️
settings/templates/admin/tipstricks.php 0% <ø> (ø) 0 <0> (ø) ⬇️
.../federatedfilesharing/templates/settings-admin.php 0% <ø> (ø) 0 <0> (ø) ⬇️
settings/templates/admin/server.php 0% <ø> (ø) 0 <0> (ø) ⬇️
apps/theming/templates/settings-admin.php 0% <ø> (ø) 0 <0> (ø) ⬇️
apps/updatenotification/templates/admin.php 0% <ø> (ø) 0 <0> (ø) ⬇️
apps/systemtags/templates/admin.php 0% <ø> (ø) 0 <0> (ø) ⬇️
apps/sharebymail/templates/settings-admin.php 0% <ø> (ø) 0 <0> (ø) ⬇️
settings/templates/admin/sharing.php 0% <ø> (ø) 0 <0> (ø) ⬇️
settings/templates/admin/additional-mail.php 0% <0%> (ø) 0 <0> (ø) ⬇️
... and 365 more

schiessle
schiessle approved these changes Apr 25, 2017
View reviewed changes
Copy link
Member

@schiessle schiessle left a comment

Choose a reason for hiding this comment

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

looks good to me after the last changes.

@karlitschek
Copy link
Member Author

karlitschek commented Apr 25, 2017 •
edited
Loading

Hmmm. I intentionally tried to group the headlines and the new description text together to create a tiny bit of structure. Now we again have a list of unconnected stuff on the page. But I don't care. At some point for the next release we should do a proper layouting of all our settings pages. So that it doesn't look randomly placed

@jancborchardt jancborchardt force-pushed the improve_settings_help branch from d1d80d6 to bbb5862 Compare April 25, 2017 15:45
@jancborchardt
Move h2 and settings-hint together
3bb41f4 
Signed-off-by: Jan-Christoph Borchardt <[email protected]>
@jancborchardt
Copy link
Member

jancborchardt commented Apr 25, 2017 •
edited
Loading

@karlitschek ok, I moved the h2 and the settings-hint back together. Looked a bit cramped with just the negative margin-top, but I added margin-bottom so it’s fine.

Also rebased on master, let’s see what the tests say.

jancborchardt
jancborchardt approved these changes Apr 25, 2017
View reviewed changes
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@jancborchardt jancborchardt added this to the Nextcloud 12.0 milestone Apr 25, 2017
@jancborchardt jancborchardt requested a review from rullzer April 25, 2017 15:50
@karlitschek
Copy link
Member Author

karlitschek commented Apr 25, 2017

looks good 👍

MorrisJobke
MorrisJobke approved these changes Apr 25, 2017
View reviewed changes
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.

Nice 👍

@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Apr 25, 2017
@MorrisJobke
Copy link
Member

MorrisJobke commented Apr 25, 2017

@karlitschek you need to sign your commits 😉

@karlitschek This is still missing. You should sign off your commits. How to do this can be found in https://github.com/nextcloud/server/blob/master/CONTRIBUTING.md#sign-your-work

@nickvergessen nickvergessen merged commit bec9f18 into master Apr 26, 2017
@nickvergessen nickvergessen deleted the improve_settings_help branch April 26, 2017 07:45
@nickvergessen
Copy link
Member

nickvergessen commented Apr 26, 2017

Merging since there is no "Schöpfungshöhe" in the commits from frank.
The git/dev setup will be fixed for future contributions in the office.

This was referenced Apr 26, 2017
Merged
Closed
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MorrisJobke MorrisJobke MorrisJobke approved these changes

@jancborchardt jancborchardt jancborchardt approved these changes

@schiessle schiessle schiessle approved these changes

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

@rullzer rullzer Awaiting requested review from rullzer

Assignees

No one assigned

Labels

4. to release Ready to be released and/or waiting for tests to finish design Design, UI, UX, etc. enhancement feature: settings

Projects

None yet

Milestone

Nextcloud 12.0

Development

Successfully merging this pull request may close these issues.

9 participants

@karlitschek @mention-bot @rullzer @jancborchardt @MorrisJobke @nickvergessen @schiessle @franktesty