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

Introduce an optional grid module to deal with a grid layout #343

Merged
merged 5 commits into from
May 15, 2024

Conversation

martin-fleck-at
Copy link
Contributor

What it does

Introduce an optional grid module to deal with a grid layout

  • Optional grid module defines a grid based on x/y coordinates
    -- Adds the grid as a background that properly zooms/resizes
    -- By default uses the GridSnapper for positioning elements
    -- Use grid for helper lines and movement tools

  • Optional debug module that shows the bounds of elements
    -- Useful for debugging but not meant for production

  • Render optional debug and grid toggles in tool palette if present

  • Add both optional modules to the workflow example

Fixes eclipse-glsp/glsp#1336

Please note this PR builds upon #342 and should only be merged afterwards!

How to test

  • See if everything still works as expected.
  • Use the new grid feature in the workflow module.

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)

If the optional accessibility module was used, the grid was derived through the GridSnapper, this needs to be replaced by a binding of the Grid.

@martin-fleck-at martin-fleck-at changed the title Issues/1336 Introduce an optional grid module to deal with a grid layout May 13, 2024
@martin-fleck-at martin-fleck-at force-pushed the issues/1336 branch 2 times, most recently from 09243d5 to 0a6a3c6 Compare May 13, 2024 16:15
Copy link
Contributor

@tortmayr tortmayr left a comment

Choose a reason for hiding this comment

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

Thanks Martin! Amazing change, I'm a big fan of the debug extension. This will be really helpful for us and our adopters!

In general, the code looks good to me.
The only thing I'm note sure about is the GridBackground service. I feel like rendering the grid background is something that should be handled by the root graph view instead.

I quickly tested this and something like

@injectable()
export class GridGraphView extends GGraphView {
    override render(model: Readonly<GGraph>, context: RenderingContext): VNode {
        const edgeRouting = this.edgeRouterRegistry.routeAllChildren(model);
        const transform = `scale(${model.zoom}) translate(${-model.scroll.x},${-model.scroll.y})`;
        return (
            <svg class-sprotty-graph={true}>
                <defs>
                    <pattern id='grid' width='10' height='10' patternUnits='userSpaceOnUse' patternTransform={transform}>
                        <path d='M 10 0 L 0 0 0 10' fill='none' stroke='gray' stroke-width='1' />
                    </pattern>
                </defs>
                <rect width='100%' height='100%' fill='url(#grid)' />
                <g transform={transform}>{context.renderChildren(model, { edgeRouting })}</g>
            </svg>
        );
    }
}

seems to do the trick.

- Optional grid module defines a grid based on x/y coordinates
-- Adds the grid as a background that properly zooms/resizes
-- By default uses the GridSnapper for positioning elements
-- Use grid for helper lines and movement tools

- Optional debug module that shows the bounds of elements
-- Useful for debugging but not meant for production

- Render optional debug and grid toggles in tool palette if present

- Add both optional modules to the workflow example

Contributed on behalf of Axon Ivy AG

Fixes eclipse-glsp/glsp#1336
- Restore usage of accessibility module
- Bind grid using TYPES.Grid instead of using local symbol
- Fix using grid in movement key tool
- Re-work rendering of grid background directly in graph

- Bonus: Add OriginViewportAction to reset to 0,0 on zoom-level 1

Contributed on behalf of Axon Ivy AG
@martin-fleck-at
Copy link
Contributor Author

@tortmayr Thank you for the quick turnaround! I pushed an additional commit that should fix all the issues that you mentioned. I also added an action to reset the viewport to 0,0 with zoom 1 as I think it is very convenient. It is basically a specialization of the more generic SetViewportCommand. I'd really appreciate it if you could have another look at it.

@tortmayr tortmayr self-requested a review May 14, 2024 14:41
- Do not create subclasses for graphs but optionally inject
- Use half-grid snapping for routing handles for nicer UX
- Avoid default grid background image that is never used
- Improve styling of tool palette now that we have broken it
Copy link
Contributor

@tortmayr tortmayr 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 improvements Martin.
Apart from a few nitpicks the change looks good to me.
I really like the improved styling of the tool palette 😉

examples/workflow-glsp/src/workflow-startup.ts Outdated Show resolved Hide resolved
packages/client/src/views/glsp-graph-view.tsx Outdated Show resolved Hide resolved
packages/client/src/features/tool-palette/tool-palette.ts Outdated Show resolved Hide resolved
packages/client/src/features/tool-palette/tool-palette.ts Outdated Show resolved Hide resolved
packages/client/src/views/glsp-projection-view.tsx Outdated Show resolved Hide resolved
packages/client/src/views/glsp-graph-view.tsx Outdated Show resolved Hide resolved
- Use GGraphView instead of GLSPGraphView as it was just an alias
- Fix order of @optional and @Inject usage
@martin-fleck-at
Copy link
Contributor Author

@tortmayr Thanks, I fixed the remaining issues!

@tortmayr tortmayr self-requested a review May 15, 2024 13:06
Copy link
Contributor

@tortmayr tortmayr left a comment

Choose a reason for hiding this comment

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

👍🏼 Looks good to me.

packages/client/src/features/grid/grid-manager.ts Outdated Show resolved Hide resolved
@tortmayr tortmayr merged commit ca7c9ae into master May 15, 2024
6 checks passed
@tortmayr tortmayr deleted the issues/1336 branch June 15, 2024 17:34
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.

Improve support for grid handling
2 participants