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

Sketch feature #45

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

PedroRodP
Copy link

Hey! I liked your plugin and really wanted to use the sketch mode, but since D2 parameters are not open to edit through an input field (this might be a nice option that allow people to play with D2 without major efforts) I forked your project and made this little enhancement. Hope it can be included in the original plugin!

@PedroRodP
Copy link
Author

Plugin with feature toggled off
image

Plugin with feature toggled on
image

@@ -53,6 +53,9 @@
class="org.jetbrains.plugins.d2.action.D2LayoutEngineActionGroup"
popup="true"
icon="AllIcons.General.Layout"/>
<action id="D2.SketchMode"
class="org.jetbrains.plugins.d2.action.D2SketchToggleAction"
icon="AllIcons.Actions.EnableNewUi"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use another icon here. Something to highlight is that the style looks hand-drawn. A random option:

image

Copy link
Author

Choose a reason for hiding this comment

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

Could you please provide this icon ID? I'm not sure I've seen it within the available IntelliJ options

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I can't provide you with a specific id. Please, take a look at the existing icons here: https://jetbrains.design/intellij/resources/icons_list/

If there are no suitable icons, consider adding one following the documentation here: https://plugins.jetbrains.com/docs/intellij/icons.html

Copy link
Author

Choose a reason for hiding this comment

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

image image

What do yo think about the options above? It's the best I could find with "hand-drawn", "hand-made", "hand", "pen", "pencil", "painting", "draw", "drawing", "sketch" keywords similar to your proposal.

Your suggested icon doesn't seem to fit right (had to resize it and cannot change color black) and it's licensed (not sure whether you want and how to properly give credits to the creator)
image

Personally, I don't like the second option and I'd go with the first one. Let me know

Copy link
Contributor

@goto1134 goto1134 Mar 20, 2024

Choose a reason for hiding this comment

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

Thank you for your research on that! For me, the pencil in the first icon looks like the best option here :)

Copy link
Author

Choose a reason for hiding this comment

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

Uploaded the first one for now

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, which one then?

Option 1 (already uploaded):
image

Option 2:
image

Option 3 (yours, has license):
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Option 1 looks best for me

Copy link
Author

Choose a reason for hiding this comment

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

Great. All set then. Thanks for your feedback!

@@ -21,6 +21,9 @@ group.D2.ThemeChooser.description=Choose a theme for D2 diagram
group.D2.LayoutChooser.text=Layout Chooser
group.D2.LayoutChooser.description=Choose a layout for D2 diagram

action.D2.SketchMode.text=Sketch
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be Sketch Mode instead?

Copy link
Author

Choose a reason for hiding this comment

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

Ok, sure

Copy link
Author

Choose a reason for hiding this comment

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

Changed

@@ -13,6 +13,7 @@ internal class D2CompilerState(
val port: Int,
val theme: String?,
val layout: D2Layout?,
val sketch: Boolean?,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we switch to a nonnull type Boolean here?

Copy link
Author

Choose a reason for hiding this comment

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

I'll give it a try. This is my first time with Kotlin 😄

Copy link
Author

Choose a reason for hiding this comment

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

Achieved 😄

internal data class D2FileEditorState(@JvmField var theme: D2Theme?, @JvmField val layout: D2Layout?) : FileEditorState {
internal data class D2FileEditorState(@JvmField var theme: D2Theme?,
@JvmField val layout: D2Layout?,
@JvmField val sketch: Boolean?) : FileEditorState {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could specify a default value here with @Default(false) and make the field not nullable

Copy link
Author

Choose a reason for hiding this comment

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

Couldn't find a suitable @default annotation, set the default with "= false"

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not familiar with kotlinx.serialization enough. Sorry for a misleading suggestion :)

The default value looks like the right solution according to the docs.

- Changed Sketch Mode text
- Made sketch Boolean non-nullable
- Changed Sketch Mode icon (pending review)
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