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

Update input helpers page #696

Closed

Conversation

ddoria921
Copy link

I started this work in response to #692.

In this PR, I started the work of converting the Input Helpers guide. I mainly worked on updating the code snippets, but also updated some of the copy where I thought it was necessary. Any feedback here is appreciated.

I particularly wasn't sure how the checkbox input helper would work with the new syntax. I assumed the correct way was to use <Input type="checkbox" />, but wasn't sure if it should be <Input @type="checkbox" /> instead.

I'm happy to make any updates that are necessary.

@@ -31,13 +31,13 @@ helper:
<tr><td>`selectionDirection`</td><td>`spellcheck`</td><td>`type`</td></tr>
</table>
Copy link
Contributor

Choose a reason for hiding this comment

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

This section can probably be deleted since you can pass literally any attributes

Copy link
Contributor

Choose a reason for hiding this comment

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

Other than @type, @value and @checked (double check the API docs PR as the final source of truth). Could replace this with an example.

unquoted, these values will be bound to a property on the template's current
If these attributes are set without the `@` symbol, their values will be set
directly on the element. However, when including the `@` symbol, these
values will be bound to a property on the template's current
rendering context. For example:
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 not quite right. It will be "bound" either way... see the API docs PR.

rendering context. For example:

```handlebars
{{input type="text" value=this.firstName disabled=this.entryNotAllowed size="50"}}
<Input type="text" @value={{this.firstName}} @disabled={{this.entryNotAllowed}} size="50" />
Copy link
Contributor

Choose a reason for hiding this comment

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

@type, disabled (no @)

@@ -48,7 +48,7 @@ current context.
To dispatch an action on specific events such as `key-press`, use the following

```handlebars
{{input value=this.firstName key-press=(action "updateFirstName")}}
<Input @value={{this.firstName}} @key-press={{action "updateFirstName"}} />
```

The following event types are supported (dasherized format):
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 the API docs PR refers to these as "custom events".. double check to make sure and try to align the language

helper to create a checkbox by setting its `type`:

```handlebars
{{input type="checkbox" name="isAdmin" checked=this.isAdmin}}
<Input type="checkbox" @name="isAdmin" @checked={{this.isAdmin}} />
Copy link
Contributor

Choose a reason for hiding this comment

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

@type, name

@@ -91,19 +91,19 @@ Which can be bound or set as described in the previous section.
Checkboxes are a special input type. If you want to dispatch an action on a certain [event](https://www.emberjs.com/api/ember/release/classes/Component), you will always need to define the event name in camelCase format:

```handlebars
{{input type="checkbox" keyPress=(action "updateName")}}
<Input type="checkbox" @keyPress={{action "updateName"}} />
Copy link
Contributor

@chancancode chancancode Mar 28, 2019

Choose a reason for hiding this comment

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

Suggested change
<Input type="checkbox" @keyPress={{action "updateName"}} />
<Input @type="checkbox" @keyPress={{action "updateName"}} />

Copy link
Contributor

@chancancode chancancode left a comment

Choose a reason for hiding this comment

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

(see inline comments)

@jenweber
Copy link
Contributor

jenweber commented Apr 2, 2019

Hi @ddoria921! Thanks for your help! It looks like these changes have been made on the master branch and not from octane. Could you branch from octane and open your PR back into octane? This PR can't be merged into Guides master yet. Thank you!

Copy link
Contributor

@jenweber jenweber left a comment

Choose a reason for hiding this comment

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

See other comment - this should target octane so that it can be merged. Thanks!

@ddoria921
Copy link
Author

@jenweber oh my bad! Should I close this PR and open a new one? I'm not sure how to keep this PR intact and branch off of octane.

@locks locks changed the base branch from master to octane April 3, 2019 07:40
@locks locks changed the base branch from octane to master April 3, 2019 07:40
@locks locks self-assigned this Apr 3, 2019
@locks locks changed the base branch from master to octane April 14, 2019 00:39
@locks locks force-pushed the update-input-helpers-guide branch from 92e0060 to 2802b7e Compare April 14, 2019 00:53
@locks
Copy link
Contributor

locks commented Apr 14, 2019

@ddoria921 I have changed the target branch and rebased for you! You will have to reset your branch, git reset --hard origin/update-input-helpers-guide and you can continue addressing Godfrey's comments. Happy editing!

@jenweber
Copy link
Contributor

@ddoria921 I would like to get your work merged so we can use it as an example that other contributors can follow in other pages of the guides. Do you have time to finish up this PR? Or would you like someone to help you with it? Thanks!

@locks locks self-assigned this May 17, 2019
@ddoria921
Copy link
Author

This is still on my radar. Gonna find some time to get this done within the next week.

guides/release/templates/input-helpers.md Outdated Show resolved Hide resolved
guides/release/templates/input-helpers.md Outdated Show resolved Hide resolved
guides/release/templates/input-helpers.md Outdated Show resolved Hide resolved
@jenweber
Copy link
Contributor

Hi @ddoria921, some of this work has been merged in through another PR, and some has not, so make sure to rebase or merge in the latest from octane before doing more work. Let me know if you need any help with this step.

Bring branch up-to-date with `octane` branch
# Conflicts:
#	guides/release/templates/input-helpers.md
@locks
Copy link
Contributor

locks commented Jun 28, 2019

@ddoria921 you still have some asks, are you able to address them or do you need help?

@jenweber
Copy link
Contributor

I'm closing this PR, as most of the work has made it into the Guides now. If you would like to include any of the other changes, feel free to open a new PR. Thanks everyone for your help!

@jenweber jenweber closed this Jul 12, 2019
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.

4 participants