CrossBreezeNL / crossmodel

An open-source logical data modeler to support the model driven data engineering approach.
GNU Affero General Public License v3.0
8 stars 3 forks source link

Provide diagram support for mapping files #46

Closed martin-fleck-at closed 8 months ago

martin-fleck-at commented 9 months ago

mapping-diagram

github-actions[bot] commented 9 months ago

Unit Test Results

  3 files  ±0   30 suites  ±0   2m 36s :stopwatch: +10s  68 tests ±0   68 :white_check_mark: ±0  0 :zzz: ±0  0 :x: ±0  207 runs  ±0  207 :white_check_mark: ±0  0 :zzz: ±0  0 :x: ±0 

Results for commit 5b9740f5. ± Comparison against base commit 0e252e77.

:recycle: This comment has been updated with latest results.

harmen-xb commented 9 months ago

Hi @martin-fleck-at,

Thanks for the update. The screens look very promising!

I will try to make time tomorrow to do a proper review.

harmen-xb commented 9 months ago

Hi @martin-fleck-at,

I am trying to test the new mapping diagram, but I keep getting the following error when opening the existing or a new mapping diagram:

image

Details: image

This error is only popping up in the UI, it's not in the browser console (or terminal).

(I did a fresh build with ´yarn clean && yarn && yarn start:browser´ after pulling the branch inside a Ubuntu Dev Container)

I also notice when running ´yarn´ the yarn.lock file is changed afterwards. This is not to be expected right?

martin-fleck-at commented 9 months ago

@harmen-xb No, the yarn.lock file should be stable. I'll try again maybe it is the different node versions, I think I set my default to 20 but I see that we are using Node 16 in the dev container so that is probably why some functions are not available. I'll have a look and try with Node 16, sorry for that!

EDIT: Yep, I can reproduce the problem and I also see the yarn.lock changes with Node 16, I'll make the PR Node 16-compatible.

harmen-xb commented 9 months ago

@martin-fleck-at I have no problem with updating it to node 20 (I guess that would be prefered anyway).

I just thought we couldn't update to node 18 or 20 because of Theia.

martin-fleck-at commented 9 months ago

@harmen-xb I pushed an update for Node 16-compatibility. We can definitely use Node 20 as Theia is also built against it (as well as Node 16 and Node 18, see https://github.com/eclipse-theia/theia/blob/master/.github/workflows/ci-cd.yml#L55) but maybe we should open a separate PR for it and see if there are more places where we could update some code and we also would want to update the Dockerfile.

harmen-xb commented 9 months ago

Hi @martin-fleck-at,

Thanks for the update, I built it again with node16 in the DevContainer and now it indeed works. It seems to work quite well with dragging, and auto-layout.

I do have some remarks:

martin-fleck-at commented 8 months ago

@harmen-xb Good catch! I pushed an update so that the sync is working again! As for the literal creation, it happens when you drag from a target attribute to an empty area on the canvas: literal-creation

We may want to re-work that if we do an overall interaction concept. The current approach is just to have a minimal-step workflow because you need to know the target attribute and the value to properly create the mapping.

harmen-xb commented 8 months ago

@martin-fleck-at Thanks for the response. For now the literal functionality is quite good I think.

There are some small things, like I can map to a target attribute multiple times now, the grammer also accepts this. This is something we will think about how this should be in the future.

For now this is definitely good enough as a first version.