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 docs-link #1130

Merged
merged 3 commits into from
Feb 11, 2022
Merged

Conversation

miguelcobain
Copy link
Collaborator

Migrates the <DocsLink/> component to a glimmer component. This makes it possible to not depend on @ember/legacy-built-in-components.

This also makes this component "officially" public by adding documentation, making it show on the docs site.
Please see inline comments.

)
)
)
as |DocsLinkTo|
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're probably wondering why the hell this is needed.

My first try was

<LinkTo
  @route={{@route}}
  @model={{@model}}
  @models={{@models}}
  @query={{@query}}
  @disabled={{@disabled}}
  @activeClass={{@activeClass}}
  @current-when={{@current-when}}
  ...attributes
  >
  {{yield}}
</LinkTo>

basically proxying every argument that <LinkTo> supports. However, this doesn't work because <LinkTo> is very picky about the arguments we pass to it.

First, we can't pass in @model and @models at the same time, even if one of them is undefined.
Then, even if you just pass @route="something" @model={{undefined}}, the link also won't work. You really need to only pass in @route="something" for those cases.

So, what I'm doing here is basically to understand what arguments were passed to <DocsLink> and then invoking the LinkTo component with only those arguments.

I think this is still worth it since:

  • we're only using public apis, so we should be fine
  • we're full glimmer components
  • we're not relying on @ember/legacy-built-in-components

So, this seems like a more future-proof approach to me.
I think <LinkTo> should validate the arguments values and not just its presence. Basically treating an undefined argument as if it wasn't passed in. That would make the implementation a lot cleaner. But that's a another discussion and maybe a good issue for the ember.js repo.

}}
<DocsLinkTo
class="docs-md__a"
@query={{or @query (hash)}}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Passing in an empty hash seems to trick <LinkTo> to behave as if nothing was passed in.
Passing in undefined triggers an error.

Copy link
Member

@RobbieTheWagner RobbieTheWagner left a comment

Choose a reason for hiding this comment

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

This looks good, thanks @miguelcobain! Nice to see we are removing @ember/legacy-built-in-components

@RobbieTheWagner RobbieTheWagner merged commit 1c4d4cd into ember-learn:master Feb 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants