🇺🇸United States @hooroomoo

Account created on 14 September 2021, over 3 years ago
  • Software Engineer at Acquia 
#

Merge Requests

More

Recent comments

🇺🇸United States hooroomoo

I think we should be able to do this purely on the frontend

🇺🇸United States hooroomoo

I've seen the same assertion fail on different MRs as well as on 0.x so I think it might just be the flaky test and there was an issue filed for it 📌 Cypress test publish-validation is flaky Active .

The fail is definitely unrelated to the frontend change. But maybe @tedbow could confirm if the fail is unrelated to the backend changes

🇺🇸United States hooroomoo

Expected because it's just updating versions but manually tested adding global css and tailwind classes in the code editor all good still in both code editor preview and layout preview.

🇺🇸United States hooroomoo

I pulled this down and tried to reproduce the error by switching the routes consecutively but couldn't.

@balintbrews theory is very solid and the code is well commented so I think it makes sense to get this in and see if it solves the issue on 0.x since there are no exact steps to reproduce it.

🇺🇸United States hooroomoo

I think the issue i was seeing in 16.2 had to do with my css aggregation being on or something else weird because I tested it again with css aggregation OFF, and I'm able to correctly see the autosaved component in my page preview. Also renders correctly after publishing.

I think this MR is good to go. I added the frontend changes to change the PATCH request to only send wanted changes and it works as expected.

This MR should fix both:

🐛 Adding component to component library results in component code and configuration being lost Active 🐛 Cannot add component to the component library using the contextual menu Active

🇺🇸United States hooroomoo

So just to preface, all my testing has been with sending PATCH requests to /xb/api/config/js_component/${id}, not the auto-save because that's what's currently in the code.

Could you confirm if client side PATCH requests to change the status or the label should still be sent to /xb/api/config/js_component/${id} or does it need to change to send it to

/xb.api/config/auto-save/js_component/${id}?

<strong>1. WITHOUT frontend change</strong>
This issue I was seeing on this MR (with no frontend changes) turned out to also exist in 0.x. The bug (let's call this Bug #1) is

<ol>
  <li>Create and add a code component to your library (using top right Add to components button in code editor)</li>
  <li>Make an edit to the code from the library</li>
  <li>Exit out of the code editor</li>
  <li>All the styling (CSS) is gone from the component in the layout preview. </li>
</ol>

Since the bug also exists in 0.x, that means this MR is not the cause of it.

<strong>2.) WITH frontend change to PATCH request to only send necessary changes.(Currently in 0.x, PATCH requests are still sending the entire component )</strong>

<ol>
  <li>Create a new code component</li>
  <li>Trigger an auto-save and then click "Add to components" from the top right in the code editor</li>
  <li>Add the component to your page preview. It doesn't render anything. </li>
  <li>If you look in your <code>/sites/default/files/astro-island

nothing is generated from your newly created code component

This behavior exists on both 0.x and this MR, WITH the frontend change to the PATCH request of only sending wanted changes.
----------------------------------------

So my conclusion: I don't think merging in this MR will cause any regressions that don't already exist in 0.x, HOWEVER, we would need to open a second bug report for the 2nd issue I described because the frontend needs to eventually change the PATCH requests to only send wanted changes.

🇺🇸United States hooroomoo

Correct, step 3 - Make any change to the code component's JS or CSS.

🇺🇸United States hooroomoo

Steps to reproduce:

1. Checkout this MR https://git.drupalcode.org/project/experience_builder/-/merge_requests/906. This MR contains frontend changes to only include desired changes in the PATCH requests AND #3519634's MR from yesterday.

2. Create a new code component
3. Make any change to the JS or CSS.
4. Click "Add to components" using the top right button in the code editor (NOT from the contextual menu)
5. After Adding it to components, the compiled JS and CSS file doesn't get generated in /sites/default/files/astro-island. When you click Add to components, the expected behavior is those files gets generated in that directory.

🇺🇸United States hooroomoo

#12. Thanks, I applied your MR 937 from 📌 Add a route for PATCHing both a config entity and its auto-saved version together Active and it fixes the issue reported in the issue summary and it shows the current auto-saved JS and CSS when I open the code editor for the component.

But when I add the component to the layout page preview, the CSS isn't there. The CSS file for the component inside of /sites/default/astro-island never gets created.

🇺🇸United States hooroomoo

🐛 Adding component to component library results in component code and configuration being lost Active is supposed to update the client PATCH requests to only include the wanted changes instead of also including the compiled/source_js in the request which I started on in this MR: https://git.drupalcode.org/project/experience_builder/-/merge_requests/9...

But currently doing that results in the component not working in the preview, I think due to something with the auto-save. I am hoping 📌 Add a route for PATCHing both a config entity and its auto-saved version together Active will unblock 🐛 Adding component to component library results in component code and configuration being lost Active which would then fix this issue.

🇺🇸United States hooroomoo

No side effects, it's just an interface. Selecting a component from the select just generates the import statement in a code block

🇺🇸United States hooroomoo

Nope sorry I was mistaken, #4 🐛 Cannot delete components from library Active doesn't apply to the Remove from components action. Marking it back to Active

🇺🇸United States hooroomoo

"remove from components" is a PATCH request, and currently, PATCH requests are sending the entire component's data over which instead of only the wanted changes. So the compiled and source JS in the request body triggers this error:

The 'imported_js_components' field is required when 'source_code_js' or 'compiled_js' is provided

I believe 📌 Add a route for PATCHing both a config entity and its auto-saved version together Active needs to be addressed first. Then we can update the PATCH request bodies to not include any of the JS in 🐛 Adding component to component library results in component code and configuration being lost Active .

🇺🇸United States hooroomoo

Testing steps with example components

1. Create a new code component named 'My Button'
2. Add this code and also add a prop named 'name' of type 'text' in the component data window

const MyButton = ({name}) => {
  return (
    <button className="bg-blue-500 hover:bg-blue-700 text-white font-bold py-2 px-4 rounded">
      {name}
    </button>
  );
};
export default MyButton;

3. Create a second code component named 'My Card'
4. Add this code:

import MyButton from '@/components/my_button'
const MyCard = () => {
  return (
    <div className="max-w-sm rounded overflow-hidden shadow-lg bg-orange-500 p-4">
      <div className="font-bold text-xl mb-2">Card Title</div>
      <p className="text-gray-700 text-base">
        Here is a simple description of what this card is about. Add anything you like here.
      </p>
      <div className="pt-4">
        <MyButton name="FOOOO"/>
      </div>
    </div>
  );
};
export default MyCard;

5. MyCard component imports the button component created in step 1 and you can pass whatever name into the name prop of the button component

🇺🇸United States hooroomoo

GIF in https://www.drupal.org/project/experience_builder/issues/3518198#comment... Set code component dependencies based on import statements Active

🇺🇸United States hooroomoo

Seeing some strange behavior after adding a code component to the library, the JS doesn't load...

🇺🇸United States hooroomoo

#34 Nevermind! The reason I was seeing client requests (PATCH requests for Rename, and Add to component) from the code component contextual menu fail was because the request always included compiled/source_js which caused the error of "missing imported_js_components property"

But because 🐛 PATCH request for config entities reset values that are not sent Active now unblocks 🐛 Adding component to component library results in component code and configuration being lost Active , the frontend can now change the PATCH requests to only include necessary changes, therefore it won't send the compiled/source_js on Rename, Add to component so the error shouldn't happen anymore.

🇺🇸United States hooroomoo

I grabbed a func over from @larowlan's poc

🇺🇸United States hooroomoo

I think we should also update the docs in ui/README.md if we want XB DDEV users to use

https://github.com/justafish/ddev-drupal-xb-dev

🇺🇸United States hooroomoo

@balintbrews I ended up with the same idea before I even saw your comments. That's validating to me haha

@effulgentsia Thanks!! Was running into a security error on my very WIP-y test commit, need to give that a try

🇺🇸United States hooroomoo

Unless are you talking about if the frontend can get the machine names of imported JS components from its source code? Then the answer is yes.

I believe there is an expectation that the author has to write imports in the JS code editor like this:

import Whatever from "@/components/button"
import FooWhatever from "@/components/heading"

Where the import statement has to be "@/components/machineName"

🇺🇸United States hooroomoo

@wim leers No it can't determine machine name from only the JS source code. What prompts this question?

Here I have a component I named "My Component" and the machine name the backend sets is "my_component" but the machine name is not in the source code.

🇺🇸United States hooroomoo

I pulled in the most recent 0.x and cannot reproduce the issue anymore. Perhaps with all the front and backend changes in the last couple of days this went away?

Will leave this open for a few more days if I or someone else ends up seeing the issue, then if not will close it as "cannot reproduce"

🇺🇸United States hooroomoo

Yep #2 is correct, it was refetching itself after the DELETE call.

🇺🇸United States hooroomoo

It looks like after the layer expands, it freezes when I try to drop the item in.

🇺🇸United States hooroomoo

@omkar-pd Are you using chrome?

Looks like it works on Chrome after testing it, but I just tested it on Firefox (my default browser) and it looks like the preview canvas is receiving the drag event and not the layers menu so the preview is scrolling instead of the layers menu.

🇺🇸United States hooroomoo

Marking stable blocker because it's a long and disruptive console warning

🇺🇸United States hooroomoo

A huge roadblock with replacing the div wrappers around the region with the HTML comments is that there is no way to initialize SortableJS (drag and drop functionality) for a region without a parent element aka if a twig template only prints out {{content}} or it has no root parent element.

This is because SortableJS requires a parent element to be passed in.

@jessebaker and I talked about this and he said that 📌 Investigate drag-and-drop solution that removes the need to drop items into the preview iFrame Active should unblock this so therefore marking this issue as postponed.

Production build 0.71.5 2024