🇺🇸United States @effulgentsia

Account created on 2 September 2006, over 18 years ago
  • Software Engineer at Acquia 
#

Merge Requests

Recent comments

🇺🇸United States effulgentsia

I don't think we want the autosave (aka preview aka draft) JS and CSS files written to disk. It would mean a bunch of short-lived files that we'd need to purge at some point. What if we define a route for those to serve them dynamically? We'd need the filename to contain the ID of the component in that case. For those, I'd think we wouldn't need the hash, since we'd want the preview to just show whatever is latest in autosave storage even if that got updated between the time that the preview HTML is sent and when the browser makes the request to the asset.

It would be great if the preview asset URL was some/path/JS_COMPONENT_MACHINE_NAME. That would make a future issue with import maps nicer. For example, we'd be able to map @/components/ to some/path/ and code in one component that does import Button from '@/components/button' would work without needing every component to be listed separately in the import map. We don't need to add anything import map related to the scope here, I'm just giving context for why some/path/JS_COMPONENT_MACHINE_NAME is preferable to some/path/HASH.js, at least for the preview/autosave case. The current behavior of HASH.js written to disk is still good for the live site (real config save) case.

🇺🇸United States effulgentsia

multiple importmap scripts is coming to browsers which would also help, but that's a way off yet

I don't think it's that far off. It's in Chrome 133 and Safari HEAD (Tech Preview). https://bugzilla.mozilla.org/show_bug.cgi?id=1916277 is the issue for Firefox, they're just choosing to block it on landing import map integrity verification first.

🇺🇸United States effulgentsia

props="{"name":"bobby"}"

Astro receives props as tuples, so this would need to be props="{"name":[0,"bobby"]}". Code Components as Block Overrides, step 1 Active contains a more generic fix for non-scalar prop values by using 'raw' instead of 0. In any case, this is pre-existing in 0.x and not this MR's responsibility to fix.

🇺🇸United States effulgentsia

This MR looks great to me. I'm not a code owner for the React code, but because I feel confident about the code being changed here and that it's fairly self contained, and because there's other work in another issue that overlaps with this, I merged it bypassing code owner approval to unblock the other work. If I made a mistake by doing this, we can revert if needed.

🇺🇸United States effulgentsia

This adds the breadcrumb block, so now this has all 4 desired for the initial scope.

🇺🇸United States effulgentsia

Note, if you're testing the wip patch, make sure to:

  • Install the xb_dev_js_blocks module (included in the patch), since that's what provides the default JS component entities.
  • Within the theme settings, click "Use Experience Builder for page templates in this theme.", since the JS component entities only override rendering of blocks that are rendered via XB dynamic components, not blocks that are rendered via Block UI entities.
🇺🇸United States effulgentsia

This includes page_title and system_branding. Still need to do breadcrumb.

🇺🇸United States effulgentsia

Here's an initial patch (yeah, I know, MRs are better, but old habits die hard) with system_menu_block implemented. I'll work on breadcrumb, site branding, and page title next.

🇺🇸United States effulgentsia

For terminology, I propose library instead of list. In other words, per #19, we'll have 4 libraries:

  1. ID=elements, label=Elements
  2. ID=extension_components, label=Components
  3. ID=dynamic_components, label="Dynamic Components"
  4. ID=primary_components, label=Components

Note that per above two different libraries can have the same label. #1 in the above list won't be in XB 1.0: that will come later when a visual component builder gets worked on.

If we follow the metaphor of Library, as in buildings with books, then libraries don't have positions, they have locations, so I also propose renaming what this MR calls position to location. This would then allow weight to be renamed to position.

Now here's where things get tricky...

On a superficial level, there are two locations: the left and the top. Continuing the metaphor with brick-and-mortar libraries, we can think of this as the main library (on the left) and satellite libraries (on the top). The area in the top bar for these satellite libraries is called tools 📌 Define the 3 areas the Top Bar will provide Active . So the above could be conceptualized as:

  1. ID=elements, label=Elements, location=tools, position=0
  2. ID=extension_components, label=Components, location=tools, position=1
  3. ID=dynamic_components, label="Dynamic Components", location=tools, position=2
  4. ID=primary_components, label=Components, location=main

On the surface, the above seems really good in terms of meeting the goals of comment #4: it would allow PHP code to add more libraries to the Tools area and have them show up without needing any change to the React code.

However, the are two problem with this approach:

  • The UX will need to intersperse other things that aren't component libraries in the Tools area. For example, we'll want a menu for managing Text styles, and we'll want this menu to be to the right of the Elements library and to the left of the Extensions library. Perhaps we could solve this by assigning documented position values for these non-library tools? So, for example, if we assign Text Styles as position=100, then #2 and #3 in the above list could be changed to position=101 and position=102.
  • Within the Extensions menu, there are other things besides the extension_components library. For example, there's also plugins. This probably makes extensions an entirely separate location from tools, since we don't want to place the extension_components library directly in the tools area, but rather inside the Extensions menu that's within the Tools area.

So that then leaves us with:

  1. ID=elements, label=Elements, location=tools, position=0
  2. ID=extension_components, label=Components, location=extensions
  3. ID=dynamic_components, label="Dynamic Components", location=tools, position=201
  4. ID=primary_components, label=Components, location=main

With the above, how much value are we even gaining from #1 and #3 being in the same location, if we need out-of-band knowledge about the position values? Would it be more robust to do:

  1. ID=elements, label=Elements, location=elements
  2. ID=extension_components, label=Components, location=extensions
  3. ID=dynamic_components, label="Dynamic Components", location=dynamic_components
  4. ID=primary_components, label=Components, location=main

But if we do the above, then we're down to a 1:1 relationship between library and location, which means we don't need location at all. We could go back to:

  1. ID=elements, label=Elements
  2. ID=extension_components, label=Components
  3. ID=dynamic_components, label="Dynamic Components"
  4. ID=primary_components, label=Components

and have the React UI hard-code where each library ID goes. Which I think is what makes the most sense given the UX we want, but it would remove the flexibility desired in comment #4.

@larowlan, @longwave: Since you were the two most wanting greater flexibility, what are your thoughts on this?

🇺🇸United States effulgentsia

By the way, the whole dance of finding all the libraries that are XB extensions seems conceptually very similar to service tags and Symfony's new !tagged_iterator. I realize that libraries.yml isn't the same as services.yml, but I wonder if we want to open a core issue to port over a similar capability/syntax to libraries.yml.

🇺🇸United States effulgentsia

I'd like @longwave to confirm that bypassing the library.discovery service is okay.

I think this is all that's left, so setting to NR and assigning to @longwave. FWIW, I think it's okay to merge this with that bypass, and open a follow up to improve that if we don't think that that bypass is a good long term solution.

🇺🇸United States effulgentsia

We likely still won't get to this in the very short term but tagging it as a stable blocker so that it stays on the radar for that. There's a chance we decide to not do any of this, or only do a subset of it, so re-titling accordingly. But either way, we should make a conscious decision before considering XB stable.

🇺🇸United States effulgentsia

Is this for URLs where we're saving the submission to autosave storage? If so, GET is incorrect HTTP semantics, not only length constrained.

🇺🇸United States effulgentsia

I think #23 is fine as an interim step, but eventually we'll need to support other CDNs. Also, eventually, we'll need the site owner to be able to control when to update versions, so I think for that we'll need the imports and their dependencies in the config entity. We can bump all that to followups though, so long as the PoC in this issue's fork doesn't get deleted, so we can refer to it later.

🇺🇸United States effulgentsia

Yes, #22 is exactly what I had mind. For #1 of that, I don't have a preference on whether to use babylon on the source or whether to compile with SWC first and then parse for imports on compiled JS. If there's no downsides to the former, then that's great.

🇺🇸United States effulgentsia

I think approach 1 is fine until we add Workspaces integration.

problem: how to prevent marking multiple as homepage in autosave data?

Do we need to? Why not just let the last save win?

🇺🇸United States effulgentsia

Config entity for storing code components Active is in, so decrementing how many issues this is blocked on.

🇺🇸United States effulgentsia

Yes to #13 but in addition there's also the desired future state where if, for example you click within the canvas on the part of the component where a prop is rendered, the ComponentInputsForm opens with focus on the corresponding widget.

🇺🇸United States effulgentsia

xb-prop-start- is not actually possible

Not in scope for 0.3, but if we want to support this at some point, I think we'd need to do it as part of the source_js -> compiled_js compilation. For example, perhaps prior to compiling with SWC, we first pass the source JS through Babylon to find where props are being output and inject the HTML comments into there.

🇺🇸United States effulgentsia

+1 to doing what the issue title says: changing 1 PageTemplate config entity per theme into N PageRegion config entities per theme.

I'm confused about what concept GlobalPageTemplate represents. If it's meant to be an override of page.html.twig, I think it should be renamed to PageTemplate (which can be a followup once this issue makes that name available). We'd need to come up with a UI (possibly after 1.0) for how to create/edit this entity. But within this UI, we could make a Region component available. Then this would be an exact replacement for page.html.twig (as well as the regions section of THEME.info.yml). I.e., someone could use this to build a new theme from scratch, including using only the XB UI to create, name, and place regions.

🇺🇸United States effulgentsia

Which would be better for enabling people to generate code components with AI , or by exporting from Figma via a Figma-to-React plugin?

🇺🇸United States effulgentsia

During a recent retrospective, the XB team identified that in addition to having a "sprint" tag, it would be helpful to have a short list of other issues that would be the most valuable to work on if/when the ones in the current sprint are all blocked or being worked on by someone else or are outside of someone's area of expertise. I'm going with "sprint candidate" as the tag for that. Often, this will be issues that are likely to be part of the next sprint, so getting them done (or at least started) early would be fantastic.

🇺🇸United States effulgentsia

I think the two should be consistent, so if we like the structure in this issue, let's open a followup to change the js_component one to match at least as far as the nesting goes, but it's okay if that one has different rules for what's required or not.

It's not reasonable to have a "code component" without CSS

When using Tailwind to style, I think most code components won't have source CSS. They'll still typically have compiled CSS though, generated from the Tailwind classes that are in the source JS.

🇺🇸United States effulgentsia

I think the initial idea for a single page_template was when we envisioned it as replacing page.html.twig entirely. Meaning, you wouldn't have the concept of theme-determined regions within XB, just a single canvas that you can lay out however you want. But since the UX evolved into being based around theme-determined regions, I think that's a fairly strong argument in favor of refactoring the config entity to match.

🇺🇸United States effulgentsia

I support this issue, but why is this a prerequisite for code components? The code components' input UX form should be IDENTICAL IN EVERY WAY as for SDCs. Is this issue really a hard blocker for that?

🇺🇸United States effulgentsia

I agree that the scope of this issue should not be to solve it for the long term; the long term is workspaces.

For the short term, I think either the ComponentSource plugin inspecting the request and loading from the autosave record instead of the config, or implementing a ConfigFactoryOverrideInterface service that inspects the request and returns overridden config so that the ComponentSource plugin can stay in its lane, is a great option. The latter is probably better in terms of purity, so if it's the same amount of work to do either, might as well do the nicer thing even if it's short lived. "Short lived" could still be a few months, depending on how long it takes us to do all the work needed to integrate Workspaces into XB. If the config override is a lot of extra work or introduces various other headaches, then it's probably not worth it, and adding a bit of ugly code into the ComponentSource plugin isn't so bad.

🇺🇸United States effulgentsia

+1 to implementing PATCH as a config save as the scope for this issue and deferring the changes needed for autosaving to Auto-save code components Active .

For when we do autosaving, I think the right semantics would still be a PATCH, but on a different resource. Instead of the canonical URL of the config entity, it would need to be the URL for the working-copy of that resource. On the back-end the working-copy is the autosave record until we have Workspaces, and when we work on integrating Workspaces we'll need to figure out when something should be an autosave record vs when it should be a config save in a Draft workspace.

Even with the CLI, we'll want the CLI patching the working-copy, not making a change to the live site. Changes to the live site should still be gated behind the "Publish All" button.

🇺🇸United States effulgentsia

Ah, so what #13 means is that we can maybe not have a scratch vs draft distinction, and just do the same thing we do for nodes, pages, and page templates: only have two concepts:

  • autosave = not an entity save = draft = what people see within XB including XB preview
  • "Publish All" = entity save = published = what people see on the actual site (outside of XB)

What that means is that content editors who are editing within XB might see an interim state of a code component (e.g., not the old design and not the new design, but whatever the person editing the code component currently has in their code editor, see #10), but we're making the decision that that is okay at this point in XB's development, because once we have Workspaces we'll be able to solve it by having the code component editor and the content editor working in different workspaces, and until then we're not expecting early XB testers to be having a content editor and a code component editor working concurrently, and even if they are, then since they're just testing XB rather than using it in production, it's okay if the content editor is seeing interim states of a code component that's actively being worked on.

I'm okay with all that. However, there's still a wrinkle that at a given point in time there can be code in the code editor that is invalid in the sense of it doesn't compile to valid JS. For example, JSX tags might not be closed or other JS syntax errors. I think we'd still want to autosave this invalid JS code, so that if the component code editor's browser crashes and they restart it, they can still pick up where they left off. But then, what do we save into the compiled_js property as part of this autosave?

  • Option 1: we save NULL (or something approximating NULL like an empty component that renders NULL) into the compiled_js property. This would then mean that content editors either see nothing within XB for that component or some placeholder message that we can insert explaining that the component's code is invalid.
  • Option 2: we retain whatever the old value for compiled_js was. Meaning, the last valid compiled code. I think this would have the better UX of showing content editors something more useful, but maybe it's weird to save an autosave record where the compiled_js property isn't an accurate reflection of the source_code_js property?
🇺🇸United States effulgentsia

"global CSS" (which perhaps needs a different name)

Yep, naming is hard. "global" here refers to "not tied to a single component". I think "global CSS" is a common term in front-end development, but "global" here isn't meant to imply back-end globality (like there only being one for the entire site, regardless of theme, etc.).

One possibility is we name the new config entity type "style_guide"? Even though we don't know exactly what this concept will entail yet, but we can start it off with a css property and find out later what other properties to add to it? And we start off with one Style Guide entity per theme, and perhaps later there'll be a product requirement to allow >1 style guide per theme?

🇺🇸United States effulgentsia

https://github.com/mozilla/standards-positions/issues/1010 is the issue to follow for updates on Firefox's plans to support importmap integrity.

🇺🇸United States effulgentsia

Are in-browser code components supposed to be theme-agnostic or should they only be available within a particular theme?

I don't even think that that's relevant. CSS for a code component will live in the code component's config entity. So the question in this issue is just whether the global CSS should be per-theme or not, and I think that's independent of whether code components are per-theme or not. @lauriii in #7 already said that global CSS is per-theme, which makes sense to me.

One problem with tying the global CSS to the page template, is then does that mean that someone who doesn't check the Use-XB-for-regions checkbox also doesn't get global CSS? Or is that not an issue anymore with 📌 PageTemplate: allow configuring which regions are exposed Active , since now we can change our logic to always create the page template and just not opt it into any regions until the user opts those regions in?

Also, in the future we may well want to have >1 page template for the same theme sharing the same global CSS, but if we don't mind moving the global CSS to a different config entity type at that point in time, when hopefully we'll have more clarity about which config entity type to move it into, then I think it's fine to put it in the page template for now as long as the paragraph above has an answer.

🇺🇸United States effulgentsia

Does @jspm/generator not do that? The CLI's jspm link can be pointed to an HTML file, but not sure whether it's only the CLI that can parse or if the generator library can as well.

Babylon seems fine if we need a separate parser. In case it helps, we'll be using https://swc.rs/docs/usage/wasm to compile. I don't know if it has any API to access the imports, but perhaps it makes sense to parse the compiled JS to find the imports, in which case something lightweight like Acorn might suffice?

🇺🇸United States effulgentsia

If we don't like .draft and .usable as the names, another pair of names could be .scratch and .draft (where "draft" is what gets used for rendering content within the XB preview, and "scratch" is what gets autosaved as someone is typing within the code editor).

🇺🇸United States effulgentsia

Preview inside Experience Builder (returned by the /api/preview endpoint) is generated with the auto-saved changes taken into account.

Is this right? My understanding was that there were actually 3 states: not just "autosaved" and "published", but also one in between the two, let's call it "usable" for now (I'm sure we can come up with a better name later).

So let's say you already have a published component, and then you start making changes to the code, these changes would get "autosaved" for the purpose of the code component maintainer not losing their work, but would not actually be seen by content editors that are using the component, even within the XB preview. When the code component maintainer is satisfied with their changes, they would click a button to update the component that's in the component library. Now these changes are in the "usable" state, meaning this is now the code that's used for rendering within the XB preview, but it's not yet "published" to the live site. Publishing to the live site would happen as part of the "Publish All" process.

The above is what I understood the designs to be the last time I saw them, but it's possible my understanding is out of date. But I would find it odd as a component code maintainer to have all my stream-of-consciousness typing into the code editor getting automatically applied to what content editors are seeing. Seems like that would also be distracting to the content editors. If I'm removing 2 Tailwind classes and adding 3 others, seems like content editors should see the old design until the new design is ready, not all the interim steps in between (one TW class removed, then the other, then one of the 3 added, etc.).

If I'm understanding correctly, then I think it could be implemented as two separate autosave values. In other words, we could have one autosave key for KEY_FOR_COMPONENT.draft and another for KEY_FOR_COMPONENT.usable. The "Update Component" button would copy from .draft to .usable and the "Publish All" button would copy from .usable to the config entity. Yeah, maybe it's a bit of a misnomer for the .usable one to be in "autosave" storage, since it's not actually autosaved (it's only saved when a button is clicked), but it lets us reuse a lot of our existing autosave infrastructure and publishing code.

With this approach, everything in 📌 Code Components should render with their auto-saved state(if any) when rendered in the XB UI Active is still accurate, so long as it's based on the .usable key, not the .draft key.

🇺🇸United States effulgentsia

Having thought about the import map problem space a bit more, I don't think it makes sense as global or per-theme config, but rather as something that gets merged dynamically based on information that's in each code component. I opened Allow code components to import from npm packages Active with more detail about that, but the point is, it means there's no other configuration that we currently know of that would be "per style guide" (see #7) other than the global CSS.

#7 says we probably don't want to combine this with the page template entity. I'm not completely clear on why we would or wouldn't want to do that, so let's go along with @lauriii's intuition on this.

Which means, we're talking about a new config entity type whose only property is the global CSS. So for naming, I guess we should go with *.global_css.yml? Possibly in the future we might figure out the details of a *.style_guide.yml and would need to update from the former to the latter, but #8 indicates that that's okay?

🇺🇸United States effulgentsia

Up until now, the XB team has been following a pseudo-scrum/pseudo-kanban process, but we're now shifting into more conventional scrum. We started a new 2-week sprint last Thursday (Jan 16). I'm tagging our current sprint's issues for visibility. This one I'm tagging with "spike" rather than "sprint", because our goal isn't to complete the implementation of it this sprint, but to figure out if the proposed approach is viable or if we need to change it in some way.

🇺🇸United States effulgentsia

Up until now, the XB team has been following a pseudo-scrum/pseudo-kanban process, but we're now shifting into more conventional scrum. We started a new 2-week sprint last Thursday (Jan 16). I'm tagging our current sprint's issues for visibility. This one I'm tagging with "spike" rather than "sprint", because our goal isn't to complete the implementation of it this sprint, but to refine its details.

🇺🇸United States effulgentsia

Up until now, the XB team has been following a pseudo-scrum/pseudo-kanban process, but we're now shifting into more conventional scrum. We started a new 2-week sprint last Thursday (Jan 16). I'm tagging our current sprint's issues for visibility. This one I'm tagging with "spike" rather than "sprint", because our goal isn't to complete the implementation of it this sprint, but to refine its details.

🇺🇸United States effulgentsia

Up until now, the XB team has been following a pseudo-scrum/pseudo-kanban process, but we're now shifting into more conventional scrum. We started a new 2-week sprint last Thursday (Jan 16). I'm tagging our current sprint's issues for visibility.

🇺🇸United States effulgentsia

Up until now, the XB team has been following a pseudo-scrum/pseudo-kanban process, but we're now shifting into more conventional scrum. We started a new 2-week sprint last Thursday (Jan 16). I'm tagging our current sprint's issues for visibility.

Production build 0.71.5 2024