Skip to content

fix: DynamicLoading story, Codec and parseXml #125

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.

Sign up for GitHub

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

Jump to bottom
Merged
tbouffard merged 4 commits into from
Oct 28, 2022
Merged

fix: DynamicLoading story, Codec and parseXml #125

tbouffard merged 4 commits into from
Oct 28, 2022

Conversation

@mayorovad
Copy link

@mayorovad mayorovad commented Oct 8, 2022

Summary

So I tried to fix DynamicLoading story and stumbled across multiple problems.

Problem 1 - parseXml not returning Document

maxGraph/packages/core/src/util/xmlUtils.ts

Lines 37 to 39 in 61ab219

export const parseXml = (xmlString: string): HTMLElement => {
return new DOMParser().parseFromString(xmlString, 'text/xml').documentElement;
};

Don't know why it's added, but original code of parseXml returning Document and not HTMLElement, which is breaking compatibility. In most cases parseXml result is used at Codec constructor, which needs Document.
Removed .documentElement.

Problem 2 - Codec.decode trying to get constructors from window

maxGraph/packages/core/src/serialization/Codec.ts

Lines 359 to 364 in 61ab219

try {
// @ts-ignore
ctor = window[node.nodeName];
} catch (err) {
// ignore
}

It can work with vanilla JS imports, but bundlers (webpack) will kill this code very easy.
Added method getCodecByName for getting constructor from CodecRegistry directly by name, It's seems to work, don't really know about side-effects of this change, need advice.

Problem 3 - graph.container.clientWidth and graph.container.clientHeight is always 0 in Storybook

I don't understand why it's like that (maybe some CSS trickery), replaced it with args.width and args.height.

Description for the changelog

Fix DynamicLoading story
Restored backward compatibility when using parseXml
Possible fix for Codec.decode method issue

Other info

DynamicLoading is fixed, but until #115 fix is merged, cells will not remove and story will look very strange 😸

@tbouffard tbouffard added the bug Something isn't working label Oct 9, 2022
tbouffard
tbouffard reviewed Oct 22, 2022
View reviewed changes
@tbouffard tbouffard mentioned this pull request Oct 22, 2022
Closed
@tbouffard
Copy link
Member

tbouffard commented Oct 22, 2022 •
edited
Loading

ℹ️ in the original mxGraph code, the getCodec function could take a string parameter.

We still have some remaining part after migration to TypeScript in Codec.decodeCell. So this method probably fails.
For the record, CodecRegistry structure is directly derived from typed-mxgraph: https://github.com/typed-mxgraph/typed-mxgraph/blob/v1.0.7/lib/io/mxCodecRegistry.d.ts

I guess we could make getCodec takes a string parameter, and benefits from alias (as I mentioned in #102), but if the proposed implementation works with getCodecByName works, let's go for it.

I will test the proposed implementation soon.

@mayorovad @tbouffard
refactor: trailing spaces in DynamicLoading story
fdc1377 
Co-authored-by: Thomas Bouffard <27200110+tbouffard@users.noreply.github.com>
tbouffard
tbouffard approved these changes Oct 23, 2022
View reviewed changes
Copy link
Member

@tbouffard tbouffard left a comment

Choose a reason for hiding this comment

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

Thanks for the fix 💪🏿.
The cells now display and new elements are added on click. As mentioned in the PR description, this story will be fully fixed when the cell deletion will work (#115)

dynamic-loading-fix-demo.mp4

@tbouffard
Copy link
Member

tbouffard commented Oct 23, 2022 •
edited
Loading

ℹ️ about the graph.container properties always set to 0, someone propose the same fix in the past: 62226e8 (#88)

This commit also proposed a fix for a typo that I have ported here: 386d09f 'fix typo when calling style.perimeter'

@tbouffard tbouffard merged commit 9428ba2 into maxGraph:development Oct 28, 2022
@mayorovad mayorovad deleted the dynamic-loading-fix branch October 29, 2022 19:19
@tbouffard tbouffard mentioned this pull request Jan 19, 2023
Closed
@tbouffard tbouffard mentioned this pull request Mar 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tbouffard tbouffard tbouffard approved these changes

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mayorovad @tbouffard