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

Fix for #123 #125

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

BarryNolte
Copy link
Collaborator

No description provided.

Copy link
Contributor

@alixander alixander 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 taking this so quick! In terms of user experience though, default shouldn't be another option. It's very confusing if someone were to be able to select "default" and "dagre", when changing the option does nothing most of the time.

Kind of related, we could have some of the options append a (default), and then a "reset to default" button. But yeah that's not for fixing this PR, just some small QOL improvement to the config page.

Effectively, this means the user can overrule display options set by the author of the D2 script, iff those overruling options are non-default. I think what you've noticed, given the solution you picked, is that users can't overrule with defaults -- and that's inconsistent. Like what if the author has chosen sketch mode but the user finds it harder to read and wants to overrule that?

Maybe we can add a checkbox next to the configs, that's like "Override user-defined configs", with a link to the docs on what that means. And then behind the scenes, the flag is passed in if the value is default and checkbox is checked, otherwise flag is only passed if value is non-default.

@BarryNolte
Copy link
Collaborator Author

Yes, the somewhat opaque nature of the default vs. CLI vs. d2-config is going to be a point of confusion.

I've thought about this since your comment and after eliminating some worse ideas, I landed back on your solution. I'll give that a try and see what you think.

@BarryNolte
Copy link
Collaborator Author

@alixander OK, one slight problem with the proposed solution, --sketch. There is no way to force sketch off. So if 'override' is chosen, and sketch is set to off in the settings, if the users document has 'sketch: true', I can not force sketch off.

@gavin-ts
Copy link
Contributor

@alixander OK, one slight problem with the proposed solution, --sketch. There is no way to force sketch off. So if 'override' is chosen, and sketch is set to off in the settings, if the users document has 'sketch: true', I can not force sketch off.

I think now you are able to run d2 --sketch=false in.d2 to turn sketch off. I don't know if that was always the case but I just tested rendering the following and it is sketched, but with --sketch=false it is not.

x
vars: {
  d2-config: {
    sketch: true
  }
}

@BarryNolte
Copy link
Collaborator Author

I can get to this after next week. I think a couple tweaks to the PR will get it to where it needs to be.

@BarryNolte BarryNolte marked this pull request as draft April 6, 2024 21:12
auto-merge was automatically disabled April 6, 2024 21:12

Pull request was converted to draft

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants