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

Make sure the style guide is always accessible to all users #39

Merged
merged 3 commits into from
Jul 25, 2019

Conversation

maxime-rainville
Copy link
Contributor

@maxime-rainville maxime-rainville commented Jul 23, 2019

This overrides the style guide controller with our own custom implementation that make the style guide publicly accessible on live environments.

Parent issue

SilverStripe\Control\Director:
rules:
'_styleguide': false
'styleguide': SilverStripe\Controllers\SimpleStyleguideController
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm moving the route where the style guide is hosted to remove the underscore.

@maxime-rainville
Copy link
Contributor Author

This is a temporary hack until benmanu/silverstripe-simple-styleguide#12 is merged.

@maxime-rainville maxime-rainville force-pushed the pulls/patch-style-guide branch from f3af1fb to 126dace Compare July 23, 2019 01:02
@bergice bergice self-assigned this Jul 23, 2019
app/_config/theme.yml Outdated Show resolved Hide resolved
@chillu
Copy link
Member

chillu commented Jul 23, 2019

I don't know what this fascination is with adding hacky overrides to project code, rather than running a fork with the pending pull requests merged in? It seems much easier to remove a fork once those pull requests are merged, rather than untangling whatever overrides we put in place (and if the same use cases are covered by the pull requests)

@maxime-rainville
Copy link
Contributor Author

I don't think it's much effort either way and I would rather avoid people coming across silverstripe/silverstripe-simple-styleguide and getting confused.

I've been burned with previous developers forking core SilverStripe module in the past to add some hacky features and a couple years down the line we're still maintaining the damn thing.

My preference is weird override over forking in most cases.

@chillu
Copy link
Member

chillu commented Jul 23, 2019

My preference is weird override over forking in most cases.

Right, but please subscribe to the pull requests to the benmanu recipe, and once they're merged, be proactive about removing those overrides, only using upstream code.

@chillu chillu merged commit b20e4e6 into silverstripe:master Jul 25, 2019
@chillu chillu deleted the pulls/patch-style-guide branch July 25, 2019 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants