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.

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

RFC 176 doc updates for ember-metal package #16154

Merged
merged 1 commit into from
Jan 30, 2018
Merged

RFC 176 doc updates for ember-metal package #16154

merged 1 commit into from
Jan 30, 2018

Conversation

karthiicksiva
Copy link
Contributor

@karthiicksiva
Copy link
Contributor Author

@locks I am not sure about how to update the reference to ember-runtime package link.

  At the heart of Ember is Ember-Runtime, a set of core functions that provide
  cross-platform compatibility and object property observing.  Ember-Runtime is
  small and performance-focused so you can use it alongside other
  cross-platform libraries such as jQuery. For more details, see
  [Ember-Runtime](https://emberjs.com/api/modules/ember-runtime.html).

https://github.com/emberjs/ember.js/blob/v2.18.0/packages/ember-metal/lib/core.js#L16

After 2.15 version of the docs, it seems ember-runtime removed (https://emberjs.com/api/ember/2.15/modules/ember-runtime). How to proceed?

toddjordan
toddjordan reviewed Jan 25, 2018
View reviewed changes
packages/ember-metal/lib/map.js Outdated
@@ -186,7 +186,7 @@ class OrderedSet {

/**
@method copy
@return {Ember.OrderedSet}
@return {OrderedSet}
@private
*/
copy() {
Copy link
Contributor

Choose a reason for hiding this comment

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

on line 220, we still have @namespace for Map. We should remove this line so Map shows up as Map in the docs and not Ember.Map

toddjordan
toddjordan reviewed Jan 25, 2018
View reviewed changes
packages/ember-metal/lib/map.js Outdated
@@ -63,7 +63,7 @@ class OrderedSet {
/**
@method create
@static
@return {Ember.OrderedSet}
@return {OrderedSet}
Copy link
Contributor

Choose a reason for hiding this comment

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

According to https://github.com/ember-cli/ember-rfc176-data/blob/master/mappings.json its still Ember.OrderedSet

toddjordan
toddjordan reviewed Jan 25, 2018
View reviewed changes
packages/ember-metal/lib/map.js
@@ -63,7 +63,7 @@ class OrderedSet {
/**
Copy link
Contributor

@toddjordan toddjordan Jan 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

We need to make the @module . @ember/map and not ember

toddjordan
toddjordan reviewed Jan 25, 2018
View reviewed changes
packages/ember-metal/lib/run_loop.js Outdated
@@ -161,7 +161,7 @@ run.join = function() {
});
```

In this example, we use Ember.run.bind to bind the setupEditor method to the
In this example, we use `@ember/runloop/bind` to bind the setupEditor method to the
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can just use bind

toddjordan
toddjordan reviewed Jan 25, 2018
View reviewed changes
packages/ember-metal/lib/watching.js Outdated
@@ -21,7 +21,7 @@ import {
invokes `Ember.propertyWillChange` and `Ember.propertyDidChange`. This is the
primitive used by observers and dependent keys; usually you will never call
this method directly but instead use higher level methods like
`Ember.addObserver()`
`@ember/object/observers/addObserver`
Copy link
Contributor

Choose a reason for hiding this comment

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

Just addObserver should do.

toddjordan
toddjordan reviewed Jan 25, 2018
View reviewed changes
packages/ember-metal/tests/accessors/get_path_test.js Outdated
@@ -3,7 +3,7 @@ import { moduleFor, AbstractTestCase } from 'internal-test-helpers';

let obj;

moduleFor('Ember.get with path', class extends AbstractTestCase {
moduleFor('@ember/object/get with path', class extends AbstractTestCase {
Copy link
Contributor

Choose a reason for hiding this comment

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

get with path

toddjordan
toddjordan reviewed Jan 25, 2018
View reviewed changes
packages/ember-metal/tests/accessors/get_properties_test.js Outdated
@@ -1,7 +1,7 @@
import { getProperties } from '../..';
import { moduleFor, AbstractTestCase } from 'internal-test-helpers';

moduleFor('Ember.getProperties', class extends AbstractTestCase {
moduleFor('@ember/object/getProperties', class extends AbstractTestCase {
Copy link
Contributor

Choose a reason for hiding this comment

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

getProperties

toddjordan
toddjordan reviewed Jan 25, 2018
View reviewed changes
packages/ember-metal/tests/is_blank_test.js Outdated
@@ -1,8 +1,8 @@
import { isBlank } from '..';

QUnit.module('Ember.isBlank');
QUnit.module('@ember/utils/isBlank');
Copy link
Contributor

Choose a reason for hiding this comment

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

isBlank

toddjordan
toddjordan reviewed Jan 25, 2018
View reviewed changes
packages/ember-metal/tests/is_empty_test.js Outdated

QUnit.test('Ember.isEmpty', function(assert) {
QUnit.test('@ember/utils/isEmpty', function(assert) {
Copy link
Contributor

Choose a reason for hiding this comment

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

isEmpty

@toddjordan
Copy link
Contributor

Generally, I'd like for functions like isEmpty to be referred to by name alone or module separate from name, like "isEmpty from '@ember/utils'". Using @ember/utils/isEmpty mixes path with function and is unclear/inaccurate.

@toddjordan
Copy link
Contributor

@Karthiick we should delete that block of documentation for Ember

@karthiicksiva karthiicksiva force-pushed the patch_embermetal_docs branch 4 times, most recently from 276182b to 7526ba1 Compare January 27, 2018 17:26
@karthiicksiva
Copy link
Contributor Author

@toddjordan I have made the changes. Let me know, if i missed something.

toddjordan
toddjordan reviewed Jan 29, 2018
View reviewed changes
packages/ember-metal/lib/map.js Outdated
@@ -2,7 +2,7 @@ import { assert } from 'ember-debug';
import { guidFor } from 'ember-utils';

/**
@module ember
@module ember/map
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to @ember/map and we are good

@toddjordan
Copy link
Contributor

@Karthiick this is great. I commented on just one more change and then I will merge.

@karthiicksiva
Copy link
Contributor Author

karthiicksiva commented Jan 30, 2018 •
edited
Loading

@toddjordan its done!

@toddjordan
Copy link
Contributor

Looks great! Will merge when the CI passes. Thanks @Karthiick !!

@toddjordan toddjordan merged commit fee0db4 into emberjs:master Jan 30, 2018
@karthiicksiva karthiicksiva deleted the patch_embermetal_docs branch January 30, 2018 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@toddjordan toddjordan toddjordan left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@karthiicksiva @toddjordan