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

paper-button: Support <a> tags with href and optional target attributes. #372

Merged

Conversation

DanChadwick
Copy link
Contributor

Also Improve dummy app page with API documentation and button. Use a-link style button to link from 1.0 dummy app to v0.2.

…utes. Improve dummy app page. Use to link to v0.2.
type: 'button',
tagName: 'button',
tagName: computed('href', function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Last time I checked using a computed tagName raises a deprecation. Hence the twiddle I mentioned in the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right you are. In wonder if there is a less ugly alternative.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Check the twiddle. I think it is the right way to do it.

Copy link
Collaborator

@miguelcobain miguelcobain May 13, 2016

Choose a reason for hiding this comment

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

Also, I think we should encapsulate the logic of testing href in a isLinkButton computed. That way, if we ever want to change the test we do it in just one place.

Copy link
Contributor Author

@DanChadwick DanChadwick May 13, 2016

Choose a reason for hiding this comment

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

Yeah, it's ugly, but maybe no alternative. When you reference a CP from init, are you guaranteed that all the parameters are already properly bound?

EDIT: I think that's fine. It's observers that don't fire from changes made in init.

Copy link
Collaborator

@miguelcobain miguelcobain May 13, 2016

Choose a reason for hiding this comment

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

I don't think it is that ugly. Init is the place to initialize variables after all.

tagName will not be bound. You can't change tagName after render. That's the reason it can't be a computed. Did I get your question right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're on the same page. Technically you should be able to use a CP without a dependent key. I'll try that, but I bet it still raises a deprecation. I just hate that init function, but it's an aesthetic thing, not a functional or technical objection.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, it also raises a deprecation with a computed with no dependent keys.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But you're right. A computed without a dependent key should not raise a deprecation, I think. We should PR ember core. :)

Yak shaving.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And bike shedding. ;)

- Don't set type=button for a-link buttons
- Use flex class rather than attribute throughout the dummy app
- Don't use a computed property for tagName
@DanChadwick DanChadwick mentioned this pull request May 13, 2016
@DanChadwick
Copy link
Contributor Author

Note: I did not create isLinkButton because this state is static, since the tagName can't change. Therefore I just set to type to null in init, along with the tagName to a. This is cleaner, simpler, smaller, and faster. If in the future we need a isLinkButton for some dynamic behavior, it too can be statically initialized in init to true.

@@ -8,12 +8,13 @@
{{/paper-toolbar}}
{{#paper-content class="md-padding demo-buttons"}}
<div class="doc-content">
<p>
<div layout="row" class="flex">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, this uses the layout attribute and a flex class. There is a layout-row I think. I've used it in the toolbar page.

<tr>
<td><strong>warn</strong></td>
<td>boolean</td>
<td>Display in the theme's warn color</td>
Copy link
Collaborator

Choose a reason for hiding this comment

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

accent, primary and warn are part of a mixin and are present in many other components. We may need to find a better way to show common attributes to avoid repeating ourselves over and over, but for now listing them here seems the best thing to do.

@miguelcobain
Copy link
Collaborator

Looks good to me. You can squash and merge (I'm on mobile). Thanks for this. 👍

@DanChadwick DanChadwick merged commit 1d3b98e into adopted-ember-addons:master May 13, 2016
@DanChadwick DanChadwick deleted the paper-button-a-link branch May 13, 2016 01:05
@openhouse
Copy link

openhouse commented May 17, 2016

So in 1.0.0-alpha.1 I can use
{{#paper-button href="/settings"}}Settings{{/paper-button}}
to get an HTML-friendly anchor tag (#371)
<a href="/settings">Settings</a>

But the raw HTML causes a hard reload which in the long run isn't ideal. It's a little slower, wouldn't allow for animations. A music player app for instance wouldn't be able to play across the transition.

I can keep the anchor tag for SEO and still use Ember's router to transition with the transition-to addon and bubbles=false. But this doesn't let me command click to open the link in a new tab.
{{#paper-button onClick=(transition-to "settings") href="/settings" bubbles=false}}Settings{{/paper-button}}

This is starting to get a little boggling compared to the {{link-to}} helper. Am I going about it wrong?

I look forward to a standard API for this that is consistent with with paper-item as well. (#207) It seems like mapping to {{link-to}} arguments would be the most Ember-intuitive way.

So grateful for this ambitious project and all your work. Thank you!

@miguelcobain
Copy link
Collaborator

@openhouse the idea was to have a link button that could link to external websites. I've never thought about different routes, but it should work. Hint: this addon renders url strings for a specific route: https://github.com/intercom/ember-href-to. Ideal to use with paper-button's new href= attribute.

@openhouse
Copy link

openhouse commented May 21, 2016

Cool!

With a minor change https://github.com/intercom/ember-href-to makes seamless transitions with cmd+click opening in a new tab.

To enable this you have to have the .href-to class

{{#paper-button href=(href-to "settings") class="href-to"}}Settings{{/paper-button}}

and this pull request.
intercom/ember-href-to#43

What this will be really great for is paper-items in the sidenav. #207

Thanks @miguelcobain for pointing me in the right direction!

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