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 2 forks source link

Add property view editor for attribute mapping #57

Closed martin-fleck-at closed 3 months ago

martin-fleck-at commented 3 months ago
github-actions[bot] commented 3 months ago

Unit Test Results

  3 files  ±0   30 suites  ±0   2m 41s :stopwatch: -1s  71 tests ±0   71 :white_check_mark: ±0  0 :zzz: ±0  0 :x: ±0  216 runs  ±0  216 :white_check_mark: ±0  0 :zzz: ±0  0 :x: ±0 

Results for commit 7b3cf680. ± Comparison against base commit 56ff42e8.

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

harmen-xb commented 3 months ago

@martin-fleck-at Is this ready to be reviewed?

martin-fleck-at commented 3 months ago

@harmen-xb Yes, sorry, forgot to add you as a reviewer as I was waiting for the CI results.

harmen-xb commented 3 months ago

@martin-fleck-at

Nice to see the property widget for the attribute mappings, this is very nice addition!

Some feedback:

martin-fleck-at commented 3 months ago
  • The fx behind the attribute, is it easy to make this another color so it's more clear it's not part of the attribute name? (maybe bit greyed out)

Done, it is now rendered in a grey color.

  • The expression editor in the widget doesn't have auto completion. Is it hard to add this logic here as well?

Adding that to the form is quite a bit more work and I suggest to do it separately since the form is not equivalent to the textual representation in other aspects as well, e.g., validation.

  • On 'Sources' when you click 'Add source' it's not very clear you can click/start typing in the new row. Is it possible to give the cell an instruction in grey or something that it's more clear. Something like "Select an attribute.."?

I improved the logic so that we get proper auto-focus on the text field and show a placeholder text like you suggested.

  • When you remove an attribute mapping by clicking on a line and pressing delete, the expression of all attribute mappings are gone. I would expect only the source attribute to be deleted.

That was a mistake indeed. I deleted all mappings that didn't have any sources but "empty" mappings are allowed.

  • The property widget doesn't show the form for attributes which don't have a source or expression. I think it should always show the form (also if there is no attribute mapping yet).

Since we allow empty mappings, I now initialize mappings with an empty attribute mapping for each target attribute. This can serve as a useful starting point for the user in the textual editor but also allows easier logic in other editors.