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

Sage 9.0.0-beta.4? #1919

Merged
merged 22 commits into from
Jul 25, 2017
Merged

Sage 9.0.0-beta.4? #1919

merged 22 commits into from
Jul 25, 2017

Conversation

QWp6t
Copy link
Member

@QWp6t QWp6t commented Jul 2, 2017

I started off just wanting to update some dependencies, but I wound up getting a little carried away. lol

Gonna work on squashing some bugs now.

To see the new post-create-project routine, run the following:

composer create-project --prefer-dist --stability=dev roots/sage:dev-update-dependencies YOUR-FOLDER-NAME-HERE

"rules": {
"no-empty-source": null
}
},
Copy link
Contributor

@oxyc oxyc Jul 2, 2017

Choose a reason for hiding this comment

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

Personal opinion, but I like having .eslintrc and .stylelintrc as separate files so that neomake and other plugins pick up the config.

Copy link
Member

Choose a reason for hiding this comment

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

seems like those tools should respect reading the configuration from package.json.. both eslint and stylelint support it

Copy link
Member Author

Choose a reason for hiding this comment

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

eslint, stylelint, browserlist, jshint, etc. (not to mention derivatives such as prettier, standard, xo, etc.) all support putting their config in package.json.

We should move Sage config to package.json.

@samrap
Copy link

samrap commented Jul 6, 2017

I noticed you tweaked stylelint to allow empty files. I submitted a PR which was merged that would preserve the comments across all framework choices when using composer create-project (#1906). It simply grabbed the first line of every sass file, emptied the file, then added the first line back to the top. You may want to revert that now, since the first line is no longer a comment and files may now be empty.

@QWp6t
Copy link
Member Author

QWp6t commented Jul 6, 2017

Already did.

6bbc676#diff-096f5c9caeaa5657ef7ea790ecc34f1b

composer create-project is handled by https://github.com/roots/sage-installer

@oxyc
Copy link
Contributor

oxyc commented Jul 7, 2017

Needs to be bumped to Webpack 3.1.0+ due to webpack-contrib/extract-text-webpack-plugin#548

app/helpers.php Outdated
* @return ContainerContract|mixed
* @SuppressWarnings(PHPMD.StaticAccess)
* @param Container $container
* @return Container|mixed
*/
function sage($abstract = null, $parameters = [], ContainerContract $container = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

@QWp6t Still have a reference to ContainerContract (removed from use)! 😃

@@ -25,28 +25,26 @@
},
"autoload": {
"psr-4": {
"Roots\\Sage\\": "app/lib/Sage/"
"App\\": "app/"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is really fine:

  • current app/ files aren't psr-4 compatible
  • App namespace may not be reused for real classes since App is specific for current app (but I may be wrong here)

Copy link
Member Author

Choose a reason for hiding this comment

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

PSR-4 doesn't limit the use of namespace in this way. PSR-4 is only for autoloading classes.

If you call a class App\Sage\Cheese, then it will look for it in app/Sage/Cheese.php, i.e., PSR-4 autoload works as expected.

"{app,resources/views}/**/*.php"
"app/**/*.php",
"config/**/*.php",
"resoruces/controllers/**/*.php",
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo! 😱 (resources)

Copy link
Member Author

Choose a reason for hiding this comment

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

lol whoops 😳

config/view.php Outdated

'namespaces' => [
// In your views use something like: @include(WC::some.view.or.partial.here)
'WC' => WP_PLUGIN_DIR.'/woocommerce/templates/',
Copy link
Contributor

@LeoColomb LeoColomb Jul 10, 2017

Choose a reason for hiding this comment

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

This is cool, but is this aimed to be included by default? Just asking 😇

Copy link
Member Author

Choose a reason for hiding this comment

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

Nah, I meant for that to be a commented out example. Thanks for catching it.

@inorbita
Copy link

Can I use this branch for production? I need this for the Foundation / JS fixes mostly.
Or perhaps any ETA when this will be merged?

@retlehs
Copy link
Member

retlehs commented Jul 24, 2017

@inorbita yes. no.

@giacomoalonzi
Copy link

@inorbita Yes, Sage 9 beta 3 is stable for production!

@QWp6t QWp6t merged commit 8691074 into master Jul 25, 2017
@QWp6t QWp6t deleted the update-dependencies branch July 25, 2017 15:58
@greatislander
Copy link

Is Beta 4 being tagged soon? I assume once #1935 is merged?

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.

9 participants