-
-
Notifications
You must be signed in to change notification settings - Fork 514
Convert core concepts / components to use gjs #2124
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
base: gjs
Are you sure you want to change the base?
Conversation
* Updated index to use gjs * Updated introduction to use gjs * Updated arguments and attributes to use gjs * Updated conditional content to use gjs * Updated block content to use gjs * Updated helper functions to use gjs * Updated component state and actions to use gjs * Updated looping to use gjs * Updated built in components to use gjs Removed ai helper
✅ Deploy Preview for ember-guides ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
guides/release/components/component-arguments-and-html-attributes.md
Outdated
Show resolved
Hide resolved
Interesting bug. Haven't seen it before. GJS codefences are powered by: https://github.com/IgnaceMaes/ember-showdown-shiki Would happily take a look at a PR there if you could find the issue. Otherwise opening an issue would also already be very helpful 😄 |
* fixed whitespace * added missing imports * fixed import paths
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! I mostly focused on the prose around the blocks since this made it easy to review.
I only made it through helper-functions but I'll be back.
Co-authored-by: Katie Gengler <[email protected]>
Co-authored-by: Katie Gengler <[email protected]>
Co-authored-by: Katie Gengler <[email protected]>
Co-authored-by: Katie Gengler <[email protected]>
Co-authored-by: Katie Gengler <[email protected]>
Co-authored-by: Katie Gengler <[email protected]>
Co-authored-by: Katie Gengler <[email protected]>
Use 'release' for links to API docs Co-authored-by: Katie Gengler <[email protected]>
|
||
You can even use SVG or web components without any changes. As long as your HTML | ||
is valid, Ember will render it. | ||
You can even use SVG or web components without any changes. As long as your HTML is valid, Ember will render it. | ||
|
||
## Self-Closing Tags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole section is super-weird, no? Anybody opposed to removing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean the self-closing tag part or this sentence?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean the entire section 'Self-Closing Tags' -- I'm not sure it is important to call out that we allow them aside from their use with components.
@@ -172,25 +182,24 @@ properties and other kinds of custom JavaScript. | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually really like the above explanation minus the spurious claims about when Ember will and won't update. The emphasis on working with the template language is important.
<template> | ||
<audio {{prop srcObject=@blob}} /> | ||
</template> | ||
``` | ||
|
||
[prop-modifier]: https://www.npmjs.com/package/ember-prop-modifier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could define the modifier here rather than refer to the addon. It is super tiny.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. The modifier implementation would be very easy. The real problem with this is it means we'd have to rewrite/rework the section below where it introduces writing a custom modifier. That wording would not make any sense.
guides/release/components/template-lifecycle-dom-and-modifiers.md
Outdated
Show resolved
Hide resolved
guides/release/components/template-lifecycle-dom-and-modifiers.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Katie Gengler <[email protected]>
Co-authored-by: Katie Gengler <[email protected]>
This PR migrates all the code blocks in the Core Concepts > Components to use GJS. In general, this means that where there were
hbs
+js
blocks, they are now a singlegjs
block. My goal here was to just do the format conversion without much content editing. There were a few things that required me to make some editorial decisions, so I was hoping somebody could give some feedback on these. Below are some notes on the conversion.General Notes
The old loose mode docs never had to worry about imports, so the location of the components relative to each other was not an issue. In the strict world, we have to import from somewhere. I did what I think was right and required the least amount of change and/or explanation. If anyone has a better approach, let me know.
I tested a majority of these changes directly in the incredible Limber tool. With very few exceptions, you can just paste the block directly in Limber and it should work. This is a wonderful resource.
GJS codefence bug?
Looks like there might be a bug in the gjs code fence, or I'm doing something wrong. Seeing
// [!code --]
at the end of several code blocks on a couple pages. See:guides/release/components/helper-functions.md
, for example. Here's a picture.Specific files
These are some notes on specific files I changed. Not married to anything here, so feel free to override everything.
guides/release/components/block-content.md
Here, I think we should use an example other than Popover or eliminate it and possibly use the
Card
example that is used later. Specifically, the problem is that the code hasthis.isOpen
but nowhere do we ever mention state orthis
. We haven't even introduced theComponent
class yet. When this documentation was written,this.isOpen
was on-the-fly tracked because ofEmber.Object
, but it's a bad example now.guides/release/components/helper-functions.md
Renamed "Global Helper Functions" to "Shared Helper Functions" and reframed it to reflect the strict mode nature of gjs.
I renamed the "Built-in Helpers" to "Built-in and Common Helpers" because nearly all of them require importing and I think "built-in" could be misleading here. We could maybe split this into two sections also.
These require importing:
No importing required:
I feel like the import vs automatically available should at least be mentioned somewhere.
Also in this section, I updated the hard-coded links from version "4.5.0" to "6.5". Do we have a relative link available here? After doing so, I realized that many of these helpers are no longer showing up in the API docs, so I am not sure what we should do. For example, the
get
helper is still there and not deprecated, so I think the API docs are just broken. Suggestions?I rewrote the section on the
in-element
helper. I think at this point in the docs, it's way too in-the-weeds since we're just introducing the helper.guides/release/components/template-lifecycle-dom-and-modifiers.md
This page has a real Frankenstein's monster quality to it. After a good intro paragraph, it goes into a philosophical reframing of things already covered elsewhere. And I believe the info to be mostly out of date for the modern Glimmer VM, e.g., "the best way to think about your component's output is to assume that it will be re-executed from the top every time anything changes in your application". This is later addressed in a CTA, stating: "Ember avoids updating parts of the DOM that haven't changed".
I converted the code snippets to gjs, but I honestly think we'd be better off removing the first 4 sections from this page:
I think these parts are generally well written, but maybe they should be extracted and folded into the other pages (like "Component Arguments and HTML Attributes" or "Conditional Content").
The "Event Handlers" section is a retread of the
<Counter>
example.The "Manipulating Properties" is easy enough to convert to gjs, but it does encourage the use of the 3rd-party,
ember-prop-modifier
, which I believe is a v1 addon. I'm not sure what the import path should be and not entirely sure how to test it. Modifiying element properties is quite easy now with modifiers and single file components, so I wonder if this part shouldn't just be rewritten to use a local modifier.guides/release/components/built-in-components.md
Converted the ember-keyboard parts to gjs, but had trouble getting them to work in Limber. I think the service is the problem, but I'm not sure. I think the import path is right. Also, perhaps the mention of
@enter
,@insert-newline
and@escape-press
for the<Input>
and<Textarea>
components should be removed.I removed the example using
{{get}}
and{{mut}}
, as the relevant dynamic property lookup is done elsewhere.