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

View improvements #364

Merged
merged 2 commits into from
Jun 20, 2024
Merged

View improvements #364

merged 2 commits into from
Jun 20, 2024

Conversation

tortmayr
Copy link
Contributor

@tortmayr tortmayr commented Jun 20, 2024

What it does

  • Refactor grind background image rendering to a css only approach

  • Use @jsx annotion for view files Use @jsx annotation instead of const JSX definition for view files. Adapt eslint config to ignore unused svg and html imports to avoid explict deactivation of the rule in each view file. Reason:

    • Sprotty base views also use annotations
    • Annotations are more robust. E.g. mocha tests with ts-node fail with an unused local error when using JSX const
  • Add tests for initializeDiagramContainer

  • Cleanup GLSPProjectionView overrides

How to test

Follow-ups

Changelog

  • This PR should be mentioned in the changelog
  • This PR introduces a breaking change (if yes, provide more details below for the changelog and the migration guide)

- Refactor grind background image rendering to a css only approach
- Use @jsx annotion for view files
  Use @jsx annotation instead of const JSX definition for view files. Adapt eslint config to ignore 
  unused svg and html imports to avoid explict deactivation of the rule in each view file.
  Reason
  - Sprotty base views also use annotations
   - Annotations are more robust. E.g. mocha tests with ts-node fail with an unused local error  
   when using JSX const

- Add tests for `initializeDiagramContainer`
- Cleanup GLSPProjectionView overrides
Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

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

Change looks good to me and can be merged as is! Great work, thank you Tobias! I only have a small question regarding the grid properties/variables but that should not prevent any merging.

backgroundPosition: `${bounds.x}px ${bounds.y}px`,
backgroundSize: `${bounds.width}px ${bounds.height}px`
// we do not set the background-image directly in the style object, because we want to toggle it on and off via CSS
[GridProperty.GRID_BACKGROUND_ZOOM]: viewport.zoom + ''
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Do you think it would be worth setting the variables in any case so they could be re-used even if the grid was not actually visible? I'm just wondering about that. We probably do not need it for the hidden rendering but for a deactivated grid someone might still use something like the zoom.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fine with me. I pushed an update

- Also add Grid properties if grid is not visible
- Adjust variable naming in default grid css
@tortmayr tortmayr merged commit 7a51d29 into master Jun 20, 2024
6 checks passed
@tortmayr tortmayr deleted the impro branch July 1, 2024 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants