Examples fixed by gelvidge · Pull Request #88 · maxGraph/maxGraph

gelvidge · 2022-06-01T04:09:51Z

Summary
Fixes majority of html examples. Some small bugs may remain but 99% ok

Description for the changelog
Updates example html files and fixes bugs in core code that were found when testing

Other info

covers #73

Finish converting core to ts, JSDoc conversion, consistency+conventio…

…examples

Examples

junsikshim

Nice work!

I had to remove NODE_OPTIONS=--openssl-legacy-provider to make it run though, people with older versions of node (< 17) would face this problem.

A couple of things to comment on.

Please run prettier on all the files so that the styles stay the same.
I think IDE configuration files should be excluded from git repository(add to .gitignore), but it's up for debate.

Thanks!

junsikshim · 2022-06-06T02:50:13Z

packages/core/src/view/GraphDataModel.ts

   */
  isLayer(cell: Cell) {
-    return this.isRoot(cell.getParent());
+    return cell != null  && this.isRoot(cell.getParent());


If cell can be null or undefined, I think it's better change the method signature,
perhaps something like isLayer(cell?: Cell).

junsikshim · 2022-06-06T03:20:27Z

packages/core/src/view/handler/SelectionHandler.ts

   * left side of the current selection. Default is false.
   */
-  guidesEnabled = false;
+  guidesEnabled = true;


If guidesEnabled should be true by default, please update the comments above. :)

junsikshim · 2022-06-06T03:21:23Z

packages/core/src/view/handler/SelectionHandler.ts

   * Maximum number of cells for which live preview should be used.  Default is 0 which means no live preview.
   */
-  maxLivePreview = 0;
+  maxLivePreview = 50;


Like above, please change the comment accordingly if the default value must change.

junsikshim · 2022-06-06T03:26:23Z

packages/html/stories/Clipboard.stories.js

-
+
+
+          for (var i = 0; i < childCount; i++)


Let's get rid of vars. :)

junsikshim · 2022-06-06T03:30:53Z

packages/html/stories/ExtendCanvas.stories.js

-  container.style.width = `${args.width}px`;
-  container.style.height = `${args.height}px`;
+  container.style.width = '321px';
+  container.style.height = '241px';


It'd be great if we can set the pixel values using storybook arguments.

tbouffard

Thanks for sharing this Pull Request. This is a lot of work.
I am pretty sure that the Pull Request contains a lot of valuable work but it is too large to review.
It also contains too much unwanted formatting changes that makes it hard to read.

There are also some logic changes that requires to be discussed because I don't understand why they are needed. I don't have time to look at all commits to try to find where they could be required.
The lib has no automatic tests, and examples don't cover all use cases. It is also very easy to fix an example and break another one. The current proposal also contains several default changes that need to be discussed.

I suggest to split the work into several smaller Pull Requests: start with examples that only require storybook files changes. That way, we can review and merge working examples quickly and have more time to discuss tricky subjects

tbouffard · 2022-06-01T06:32:30Z

package.json

    "webpack-merge": "^5.8.0"
  },
  "sideEffects": true
+


Extra change

tbouffard · 2022-06-01T06:33:17Z

packages/core/src/serialization/Codec.ts

  decode(node: Element, into?: any): any {
+
+
+    //****Edit note - there may be other classes needed here????******


tbouffard · 2022-06-01T06:34:57Z

packages/core/src/serialization/Codec.ts

        // @ts-ignore
-        ctor = window[node.nodeName];
+        ctor = classMap[node.nodeName]
+        if (ctor == undefined) console.log(node.nodeName)


Please do proper Error management

tbouffard · 2022-06-01T06:35:28Z

packages/core/src/types.ts

  imageBorder?: ColorValue;
  imageHeight?: number;
  imageWidth?: number;
+  imageVerticalAlign?: VAlignValue;


For indicator only? There was no need for this in the original mxGraph code, why do we need this now?

tbouffard · 2022-06-01T06:36:24Z

packages/core/src/util/cloneUtils.ts

        (transients == null || transients.indexOf(i) < 0)
      ) {
-        if (!shallow && typeof obj[i] === 'object') {
+       if (!shallow && typeof obj[i] === 'object') {


Extra change + formatting issue

tbouffard · 2022-06-07T04:21:57Z

packages/html/package.json

  "main": "index.js",
  "scripts": {
-    "dev": "start-storybook -p 8901"
+    "dev": "cross-env NODE_OPTIONS=--openssl-legacy-provider start-storybook -p 8901 ./public"


Please revert this: see #74 and #64 (reply in thread)

tbouffard · 2022-06-07T05:08:03Z

packages/core/src/util/Utils.ts

  }
 };

+//getBBox of SVG element before rendered in DOM


please write proper JSDoc

tbouffard · 2022-06-07T05:11:59Z

packages/core/src/view/canvas/SvgCanvas2D.ts

        bbox = new Rectangle(
-          (x + 1) * s.scale,
+          x  * s.scale,
          (y + 2) * s.scale,
-          w * s.scale,
+          (w +1) * s.scale,
          (h + 1) * s.scale
        );


I am pretty sure the previous implementation cames from the orignal mxGraph.
There are a lot of place where there are +1 on position, width or height. It is very hard to do the review here. Unfortunately, I don't have time to dig into the 56 commits to find which example may need the change and how this change impacts other examples.

tbouffard · 2022-06-07T05:13:46Z

packages/core/src/view/mixins/ConnectionsMixin.ts

    if (point) {
-      perimeter = edge.style[source ? 'exitPerimeter' : 'entryPerimeter'] || false;
-
+      perimeter = edge.style[source ? 'exitPerimeter' : 'entryPerimeter'] || true;


please don't change defaults

tbouffard · 2022-06-07T05:14:31Z

packages/core/src/view/mixins/FoldingMixin.ts

 >;
 type PartialType = PartialGraph & PartialFolding;
-
+Client.imageBasePath = '/images'


You are changing the configuration here and this impacts all users of the lib, not only the example.

gelvidge · 2022-06-07T09:24:08Z

Thanks both for reviewing and thanks for the feedback. I'll look into seeing how the PR can be improved by splitting and tidying up some of the formatting

tbouffard · 2022-06-08T07:05:19Z

@gelvidge hi, I will have some time on Thursday (June, 9th) to work on the project. I can try to cherry-pick some commits that quickly fix the examples in a dedicated PR.
Please let me know

aminosbh · 2022-06-17T10:55:14Z

@gelvidge we are currently on the process of changing the license of this project to the standard Apache 2.0 license (see #89). Do you agree to use the Apache 2.0 license on your contribution ?

gelvidge · 2022-06-20T23:09:25Z

Yes that is OK. I will aim to get around to submitting the new commits soon but all my contributions can be under Apache v2

tbouffard · 2022-10-23T07:38:07Z

@gelvidge hi, do you still plan to recreate some PR based on the fixes proposed here?
@mayorovad started fixing some stories and some are already merged. That's why there is some merge conflicts here.

So I suggest you synchronize together if you both want to set new stories.

tbouffard · 2024-04-12T07:39:34Z

This PR is kept opened on purpose even if it won't be merged. There is no need to update it, nor rebase it, as it it too large.

However, it contains a lot of valuable information and fixes. It can be used as a source of information to create new focused and small PR about a single topic.

tbouffard · 2024-10-13T15:17:43Z

Closing this PR.
It will be used to pick some changes when fixing remaining problems in stories. See #505.

gelvidge added 30 commits January 8, 2022 09:53

Merge pull request #2 from maxGraph/development

eed5652

Finish converting core to ts, JSDoc conversion, consistency+conventio…

fix error caused by undefined in getSelectionCell

32f5c19

fixed anchors example

b79ac4b

updated references style.edge to style.edgeStyle

1179e26

fixed swimlanes

8546ca9

fixed bugs in orgchart example

f7743a6

fixed collapse example

f117758

fixed tree story

fa6ee48

fixed autolayout

c9eb40e

fixed boundary

560a33a

Fixed guides example

eac9d3e

Fixed Dragsource example

ad616f0

Fixed stylesheet example

a1abe79

cleaned up some debugging changes

0d789a0

fixed helloports example

5278f8b

fixed orthogonal example

fc8ed77

fixed portrefs example

2ec2d18

fixed overlays example

a6dba2f

fixed autolayout example

f98ee5e

fixed nw cell handle causing drag instead of resize

c6a1299

Merge branch 'development' into examples

ae11bc4

fixed type check error

92b2d6a

Merge branch 'examples' of https://github.com/gelvidge/maxGraph into …

7b6b959

…examples

Merge pull request #4 from gelvidge/examples

51e7206

Examples

fixed filename case and fixedpoints example

59f764e

fixed contexticons example

0bdae3b

fixed control example

f93db32

fixed hovericons example

e2ae0aa

fixed images example

3369322

fixed indicators example

89c5a95

gelvidge added 6 commits June 1, 2022 10:24

file name updates

80f8a1d

file name updates

bbc183a

File casing updates

9aae8fa

fixed some bugs as aresult of styling change from string to object

cc91fda

bug fixes

761fb3f

fixed typescript build error

d44b881

junsikshim reviewed Jun 6, 2022

View reviewed changes

tbouffard requested changes Jun 7, 2022

View reviewed changes

tbouffard mentioned this pull request Oct 9, 2022

chore: configure eslint #123

Merged

tbouffard marked this pull request as draft October 23, 2022 07:38

tbouffard mentioned this pull request Dec 13, 2022

fix: consider default style when computing effective style properties #152

Merged

tbouffard mentioned this pull request Jan 19, 2023

Decoding XML breaks when code is minified #169

Closed

tbouffard added the bug Something isn't working label Dec 3, 2023

tbouffard changed the base branch from development to main February 15, 2024 09:16

tbouffard mentioned this pull request Apr 12, 2024

refactor: GraphDataModel.isLayer signature accepts null Cell #389

Merged

tbouffard mentioned this pull request Apr 17, 2024

fix: correctly detect parent in SwimlaneMixin.dropTarget #390

Merged

tbouffard mentioned this pull request Jun 21, 2024

fix(story): improve the 'Stencils' story #476

Merged

hbmartin mentioned this pull request Jul 3, 2024

docs: improve setup instructions for js-example #486

Merged

tbouffard mentioned this pull request Aug 17, 2024

Ensure that the stories work as the mxGraph examples #505

Open

tbouffard mentioned this pull request Sep 12, 2024

fix: make the OrgChart story better work #532

Draft

9 tasks

reuse-ay mentioned this pull request Sep 21, 2024

fix(story): convert page-breaks story to typescript and add missing imports #537

Merged

7 tasks

reuse-ay mentioned this pull request Oct 2, 2024

fix(PrintPreview): Execute writeHead even if css is null #541

Merged

7 tasks

tbouffard closed this Oct 13, 2024

		decode(node: Element, into?: any): any {


		//**Edit note - there may be other classes needed here????****

Conversation

gelvidge commented Jun 1, 2022 • edited by tbouffard Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

junsikshim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbouffard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbouffard Jun 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gelvidge commented Jun 7, 2022

Uh oh!

tbouffard commented Jun 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aminosbh commented Jun 17, 2022

Uh oh!

gelvidge commented Jun 20, 2022

Uh oh!

tbouffard commented Oct 23, 2022

Uh oh!

tbouffard commented Apr 12, 2024

Uh oh!

tbouffard commented Oct 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gelvidge commented Jun 1, 2022 •

edited by tbouffard

Loading

tbouffard Jun 7, 2022 •

edited

Loading

tbouffard commented Jun 8, 2022 •

edited

Loading