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

Add check for 'HTMLElement' type #80

Merged
merged 15 commits into from
Dec 2, 2020
Merged

Conversation

KevinBatdorf
Copy link
Contributor

@KevinBatdorf KevinBatdorf commented Nov 27, 2020

Fixes #78

Serializes HTMLElement types and changes the logic on what is readOnly and what readOnly can do.

readOnly only affects editing now, not opening/closing

@HugoDF
Copy link
Collaborator

HugoDF commented Nov 27, 2020

Re tests let's chat in #81

@HugoDF HugoDF added this to the v0.0.8 [unreleased] milestone Nov 27, 2020
@HugoDF
Copy link
Collaborator

HugoDF commented Nov 28, 2020

Btw the project can now be tested with Cypress when changing backend.js or the Alpine.js Panel app.

I've stuck the test for this PR on the todo list at #86 but we can add it here as well.

@HugoDF
Copy link
Collaborator

HugoDF commented Dec 1, 2020

Hmmm on second thought, it looks like we need to rethink the way we pass "readonly" attributes to the panel, see the screenshots, el was set as el: $el in x-data, it's editable

Screenshot 2020-12-01 at 18 42 31
Screenshot 2020-12-01 at 18 42 36

We'll need to make HTMLElement fields readOnly in the place that processes the received components

readOnly: type === 'function',

so in short:

  1. let's extract that function (existing comment) that replaces non-serializable things (things of type function become 'function', instances of HTMLElement become 'HTMLElement')

  2. let's do readOnly: type === 'function' || (value === 'HTMLElement' && type === 'object') in flattenSingleAttribute

Note that this PR fix will only work for top-level elements, not elements inside of Arrays/Objects, we should probably deal with that separately though.

@KevinBatdorf
Copy link
Contributor Author

I think this turned out well. Can you take a look when you have time? I can add some tests later this week. I feel I have a pretty decent grasp as to how the codebase works now :)

devtools

Also, I noticed in the simulator the arrows aren't showing. I think we just need to copy over some CSS that's inlined. I wasn't sure so left it be. We can do it in another branch easily enough though.

Copy link
Collaborator

@HugoDF HugoDF left a comment

Choose a reason for hiding this comment

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

Nice one on the element name, just a few questions.

Have you tried it out with els: [$el]? Just wondering if it magically works otherwise I think this is good to merge and we can create a separate issue to deal with HTML elements in nested object/arrays

Edit: I've tried with els: [$el] and we end up with the same error as the initial issue for this, something about cyclic values, let's raise as a separate issue #96

packages/shell-chrome/src/backend.js Show resolved Hide resolved
packages/shell-chrome/src/utils.js Show resolved Hide resolved
@HugoDF
Copy link
Collaborator

HugoDF commented Dec 2, 2020

Also, I noticed in the simulator the arrows aren't showing. I think we just need to copy over some CSS that's inlined. I wasn't sure so left it be. We can do it in another branch easily enough though.

Fixed this in #95

@HugoDF HugoDF force-pushed the fix/detect-element-data-type branch from c99f400 to b8978bb Compare December 2, 2020 17:04
@HugoDF HugoDF merged commit 0ce5fff into master Dec 2, 2020
@HugoDF HugoDF deleted the fix/detect-element-data-type branch December 2, 2020 17:09
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.

Circular structure bug when referencing the $el in a different variable
2 participants