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

useMaxWidth flag doesn't really use the max width #5038

Open
sidharthv96 opened this issue Nov 16, 2023 · 2 comments
Open

useMaxWidth flag doesn't really use the max width #5038

sidharthv96 opened this issue Nov 16, 2023 · 2 comments
Labels
Status: Triage Needs to be verified, categorized, etc Type: Bug / Error Something isn't working or is incorrect

Comments

@sidharthv96
Copy link
Member

sidharthv96 commented Nov 16, 2023

Description

These are the description and implementation of useMaxWidth.

When this flag is set to true, the height and width is set to 100% and is then scaled with the available space. If set to false, the absolute space required is used.

  if (useMaxWidth) {
    attrs.set('width', '100%');
    attrs.set('style', `max-width: ${width}px;`);
  } 

As we can see in the implementation, width is set as 100%, but an extra max-width attribute is added, which actually prevents the diagram from filling up the available space, as mentioned in the description.

Current implementation, where the diagram does not actually fill up the space.

image

After removing the max-width style.

image

If useMaxWidth = false, with viewport size smaller than the diagram.

image

So, useMaxWidth is mandatory for shrinking, but breaks growing (after the width of the diagram).

This could be the underlying reason for issues like this also.

image

Suggested Solutions

We need to handle this, maybe as a breaking change (by removing the max-width style), or by deprecating useMaxWidth, and adding a new responsive flag or something.

@sidharthv96 sidharthv96 added Type: Bug / Error Something isn't working or is incorrect Status: Triage Needs to be verified, categorized, etc labels Nov 16, 2023
@knsv
Copy link
Collaborator

knsv commented Nov 16, 2023

I think there are a few cases:

  • You want the diagram not to be resized
  • You want to use the existing space, limiting the size of the diagram to the constraints of the container.
  • You don't want to resize the diagram unless it is bigger than the container. This is how useMaxWidth is supposed to work. When including diagrams on a web page/document this is a common preference.

I like the suggested responsive flag, let's call it 'scaling' with the legal values 'none', 'fit-container' and 'shrink-to-fit-container'.

I think the view box is set in inconsistent ways which could explain weird scaling for some diagrams.

@jgreywolf jgreywolf added include roadmap items to add to roadmap for auto workflow and removed include roadmap items to add to roadmap for auto workflow labels Nov 16, 2023
@sidharthv96
Copy link
Member Author

#5100 is another example. After I added the fix in #5102, the diagram will shrink only if the useMaxWidth is true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Triage Needs to be verified, categorized, etc Type: Bug / Error Something isn't working or is incorrect
Projects
None yet
Development

No branches or pull requests

3 participants