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

WIP - Octane Guides #455

Merged
merged 607 commits into from
Dec 20, 2019
Merged

WIP - Octane Guides #455

merged 607 commits into from
Dec 20, 2019

Conversation

mansona
Copy link
Member

@mansona mansona commented Feb 14, 2019

This PR is intended to be a "tracking PR" to show the differences that the octane branch has from master

This is not going to be merged until Octane is officially released 👍

The Netlify Preview App that is associated with this PR will be the official demo of the new Octane Guides: https://deploy-preview-455--ember-guides.netlify.com/release/

@MelSumner
Copy link
Member

Thank you for setting this up!

@mike-north
Copy link
Contributor

@lisaychuang - as a newcomer to ember, can you go through the guides and provide feedback please?

object and Ember Data will send them along with each Ajax request.
(Note that we set headers in `init()` because default property values
object and Ember Data will send them along with each ajax request.
(Note that we set headers in `constructor()` because default property values
Copy link
Contributor

@mike-north mike-north Mar 11, 2019

Choose a reason for hiding this comment

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

this is no longer true w/ ES6 classes.

class ApplicationAdapter extends JSONAPIAdapter {
  headers = { ANOTHER_HEADER: 'Some header value' }
}

will not place anything on the ApplicationAdapter prototype

@@ -30,15 +30,15 @@ ember generate component button-with-confirmation

We'll plan to use the component in a template something like this:

```handlebars {data-filename=app/templates/components/user-profile.hbs}
```handlebars {data-filename=src/ui/components/user-profile/template.hbs}
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be reverted, in light of MU project layout not shipping with octane?

Copy link
Contributor

Choose a reason for hiding this comment

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

cleanup issue: #584

@@ -0,0 +1,99 @@
In a component template you can display two types of value,
properties of the component object,
or values passed into the component.
Copy link
Contributor

Choose a reason for hiding this comment

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

grammar/clarity issues

properties of the component object,
or values passed into the component.

To use a property you prefix it with `this`, like so:
Copy link
Contributor

@mike-north mike-north Mar 11, 2019

Choose a reason for hiding this comment

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

Suggested change
To use a property you prefix it with `this`, like so:
To use a property, prefix it with `this` like so:

super(...arguments);
}

@tracked
Copy link
Contributor

Choose a reason for hiding this comment

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

needs an import to make this example complete

headers: computed('session.authToken', function() {
export default class ApplicationAdapter extends JSONAPIAdapter {
@service session;
@tracked session.authToken;
Copy link
Contributor

Choose a reason for hiding this comment

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

invalid syntax

using native JavaScript methods like
[fetch](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API)
Some developers write all their own code to handle API requests,
using native JavaScript methods like
Copy link
Contributor

@mike-north mike-north Mar 12, 2019

Choose a reason for hiding this comment

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

Suggested change
using native JavaScript methods like
using native browser APIs like

reword so as not to imply that all JS environments provide fetch at this time

normalize(typeHash, hash) {
hash['songCount'] = hash['song_count']
delete hash['song_count']
return this._super(typeHash, hash);
return super(typeHash, hash);
Copy link
Contributor

Choose a reason for hiding this comment

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

elsewhere it seems like ...arguments is used. Why are we doing things differently here?


export default Route.extend({
export default class ApplicationRoute extends Route {
@service store;
model() {
this.store.pushPayload({
Copy link
Contributor

Choose a reason for hiding this comment

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

kind of odd that this model hook isn't returning anything. Wouldn't we want to return what we pushed?

import $ from 'jquery';

export default Route.extend({
export default class ConfirmPaymentRoute extends Route {
@service store;
actions: {
confirm(data) {
$.ajax({
Copy link
Contributor

Choose a reason for hiding this comment

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

might be a good time to update this to fetch, and potentially async/await


## Handling Action Completion

Often actions perform asynchronous tasks, such as making an ajax request to a
Copy link
Contributor

Choose a reason for hiding this comment

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

ajax HTTP b/c it's 2019 💥

To define a component, run:
Component definitions consist of:

1. A template
Copy link
Contributor

Choose a reason for hiding this comment

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

If @glimmer/component@0.14.0-alpha.3 is an indication, templates are optional too, and default to {{yield}} (or have the equivalent effect) just like we're used to w/ @ember/component

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually tested this out with @ember/component in Ember Twiddle, and removing the template didn't default to {{yield}}, it defaulted to nothing, which was why I assumed that this was the case with glimmer components as well.

Copy link
Contributor

@mike-north mike-north Mar 12, 2019

Choose a reason for hiding this comment

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

interesting. I tested this in a local app before making this comment. Maybe twiddle and real apps are out of alignment?

- Component templates to exist in `app/templates/components/`
- Component classes to exist in `app/components/`

Ember looks for files that are the `kebab-case` version of your component name.
Copy link
Contributor

Choose a reason for hiding this comment

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

may want to call this "dasherize-case" for consistency with @ember/string utilities


#### Generating a Component

You can create these files automatically by generating a component using the
Copy link
Contributor

Choose a reason for hiding this comment

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

remove "the"

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a point of consistency, I believe we refer to it as "the Ember CLI" elsewhere in the docs. Happy to change it, but I do think we should be consistent

Copy link
Contributor

@mike-north mike-north Mar 12, 2019

Choose a reason for hiding this comment

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

I'd consistently stick with the conventions in the Ember CLI guides, which are aligned with "using Ember CLI"


We'll talk more about arguments in [the next
section](../arguments-and-attributes). All arguments are prefixed with the `@`
symbol, so whenever you see `{{@...` you know its referring to any argument.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is at least the third distinct place I've seen {{this.foo}} vs {{foo}} vs {{@foo}} discussed. We're in need of a single great explanation we can refer back to repeatedly.

parameter we've provided to the function specified. So now when the action is
invoked, that parameter will automatically be passed as its argument,
effectively calling `sendMessage("info")`, despite the argument not appearing in
the calling code.
Copy link
Contributor

Choose a reason for hiding this comment

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

This explanation could benefit from an "equivalent JS" example that may be easier for people to understand than closures, partial application, etc...

(...args) => this.sendMessage("info", ...args);

}
```

In order for `confirmValue` to take on the value of the message text, we'll bind
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 a little concerned about too much overloading of the term "bind". It could mean (and is used in the guides to refer to each of the following)

  • data-binding
  • creation of a new variable binding (i.e., {{#let apple as |grape|}} or const grape = apple)
  • Function.prototype.bind

To avoid confusion, we may want to take care to differentiate very clearly

```

We can tell the action to invoke the `sendMessage` action directly on the
messaging service with the `target` attribute.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a target here

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be removed, yeah. The action is now bound directly on the service, so target is not needed.

}
```

Then our `system-preferences-editor` template passes its local `deleteUser`
Copy link
Contributor

@mike-north mike-north Mar 12, 2019

Choose a reason for hiding this comment

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

I'd avoid the use of "local" here since it clashes with the description of "local state" in the state management section

{{#each this.myPosts as |post|}}
{{!-- either foo-component or bar-component --}}
{{#let (component post.postType) as |Post|}}
<Post @post={{post}} />
Copy link
Contributor

Choose a reason for hiding this comment

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

you have @post post and Post all in scope here. May be worth picking more distinct variable names for clarity.

@jenweber
Copy link
Contributor

If we ever merge this, we need to revert the following changes:

  • hiding the edit pencil and the search bar
  • changes to app/router for Google analytics
  • changes to robots.txt that prevented crawling the staging app

Chris Garrett and others added 26 commits December 15, 2019 09:18
Updating the octane branch with the last few releases
* Fix `rel` attributes

The values should be separated by space instead of comma.

---

Commit:  ember-learn/super-rentals-tutorial@24dcc4b
Script:  https://github.com/ember-learn/super-rentals-tutorial/blob/24dcc4b11ad9c19e2f39153af36110d75db8c26f/.github/workflows/build.yml
Logs:    https://github.com/ember-learn/super-rentals-tutorial/commit/24dcc4b11ad9c19e2f39153af36110d75db8c26f/checks
(cherry picked from commit 1e091df)

* [BUGFIX] remove accidental word in octane guide

Deleted an extra "do" in the Zoey says in the responding to user interactions with actions section. Changed " forget to do add" to "forget to add".

---

Commit:  ember-learn/super-rentals-tutorial@9962c95
Script:  https://github.com/ember-learn/super-rentals-tutorial/blob/9962c95a677159f526c30641ca4fb7a6425a6fb0/.github/workflows/build.yml
Logs:    https://github.com/ember-learn/super-rentals-tutorial/commit/9962c95a677159f526c30641ca4fb7a6425a6fb0/checks
(cherry picked from commit 9554a49)

* remove references to octane blueprint

---

Commit:  ember-learn/super-rentals-tutorial@9dc125d
Script:  https://github.com/ember-learn/super-rentals-tutorial/blob/9dc125d801e78924dac35cda931bfa469f4a8097/.github/workflows/build.yml
Logs:    https://github.com/ember-learn/super-rentals-tutorial/commit/9dc125d801e78924dac35cda931bfa469f4a8097/checks
(cherry picked from commit e0fb19a)

* [CRON] Thursday Dec 19, 2019

---

Commit:  ember-learn/super-rentals-tutorial@785115a
Script:  https://github.com/ember-learn/super-rentals-tutorial/blob/785115a55168b2871d76072a465b9acbfeb6e0c8/.github/workflows/build.yml
Logs:    https://github.com/ember-learn/super-rentals-tutorial/commit/785115a55168b2871d76072a465b9acbfeb6e0c8/checks
(cherry picked from commit 86ebb80)

* Manual edits
Co-Authored-By: Robert Jackson <me@rwjblue.com>
Co-Authored-By: Robert Jackson <me@rwjblue.com>
Co-Authored-By: Robert Jackson <me@rwjblue.com>
Co-Authored-By: Robert Jackson <me@rwjblue.com>
Co-Authored-By: Robert Jackson <me@rwjblue.com>
Co-Authored-By: Robert Jackson <me@rwjblue.com>
Co-Authored-By: Robert Jackson <me@rwjblue.com>
Co-Authored-By: Robert Jackson <me@rwjblue.com>
Co-Authored-By: Robert Jackson <me@rwjblue.com>
Co-Authored-By: Robert Jackson <me@rwjblue.com>
Co-Authored-By: Jen Weber <weberj10@gmail.com>
Co-Authored-By: Jen Weber <weberj10@gmail.com>
Co-Authored-By: Jen Weber <weberj10@gmail.com>
Merge this just before merging `octane` branch - important infra updates
upd(configure:ember): add writeup on default-async-observers
@jenweber jenweber marked this pull request as ready for review December 20, 2019 18:59
@jenweber jenweber merged commit 13f467e into master Dec 20, 2019
@jenweber
Copy link
Contributor

It's merged! A zillion thanks to our zillion contributors who have done work for over a year on on these new guides.

The guides should be live in about half an hour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet