search

dynamic / silverstripe-maintenance-mode

Maintainance/Offline Mode Module for SilverStripe
BSD 2-Clause "Simplified" License
21 stars 18 forks source link

Update MaintenanceModeExtension.php #25

Closed martinduparc closed 8 years ago

martinduparc commented 8 years ago

Better layout for the Settings/access tab

mattrayner commented 8 years ago

Hi Martin, Thanks for your pull request!

Our concern about this is that, although it's more concise, we feel it makes the code less readable - potentially increasing the barrier to entry for developers that are less experienced?

Other than for conciseness, is there any other reason you feel this should be merged?

Thanks, Matt

martinduparc commented 8 years ago

Hi Matt,

The main problem this PR is fixing is layout in the CMS. Here is before: before

Here is after: after

We can definitely make the code more readable by adding line breaks if it helps.

Thanks!

mattrayner commented 8 years ago

Hi Martin, I see what you mean now!

If you could break the code up to make it a little more readable we'll happily merge in!

dljoseph commented 8 years ago

Perhaps you could simply change the $headingLevel = 3 in the original code to $headingLevel = 4 - that will make it fit and break up the layout enough to make it stand out as a separate section. Thoughts?

martinduparc commented 8 years ago

@mattrayner I've updated my branch wth additional line breaks.

@dljoseph I think it integrates better if it follows the existing layout but I get what you're saying. I've also tried to follow what SilverStripe does "natively". See https://github.com/silverstripe/silverstripe-cms/blob/master/code/model/SiteTree.php#L1922 and https://github.com/silverstripe/silverstripe-cms/blob/master/code/model/SiteTree.php#L1950

mattrayner commented 8 years ago

@martinduparc Thanks for making those changes - merged appreciatively 👍