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

Generalize columns #51

Merged
merged 9 commits into from
Oct 4, 2024
Merged

Generalize columns #51

merged 9 commits into from
Oct 4, 2024

Conversation

greimel
Copy link
Collaborator

@greimel greimel commented Sep 18, 2024

closes #37. This is based on @fonsp 's suggestion in #37.

You can now display any number of columns using Columns(a, b, c, d, e). Columns will have equal width by default.

You can change the width using Columns(a, b, c, widths = [30, 20, 50]) (widths will be normalized to sum to 100 internally), and you can also customize the gap between columns.

Unfortunately, I couldn't figure out how to make text boxes fill the whole column:
image

The warning box is just as wide as the text.

Any ideas how to make the box fill up the whole column?

@greimel
Copy link
Collaborator Author

greimel commented Sep 18, 2024

@fonsp Is DivElement considered public API and/or stable? Is it safe to use it here?

@greimel greimel force-pushed the columns branch 6 times, most recently from 6926d24 to 72cd913 Compare September 18, 2024 11:13
@greimel greimel force-pushed the columns branch 2 times, most recently from 06df789 to fc1d44b Compare September 18, 2024 13:17
@greimel greimel changed the title Generalize columns Generalize columns Sep 18, 2024
@eford
Copy link
Collaborator

eford commented Sep 20, 2024

  1. It would be nice to hear from @fonsp before deciding whether we can expect this to be robust to future versions of PlutoUI.
  2. Since the existing implementation does result in text boxes having the correct size, there is some motivation for keeping the original implementation. Is there a problem that this PR solves that outweights that?

@fonsp
Copy link
Member

fonsp commented Oct 2, 2024

Hey all! Yes you can use it, and you should use it! I think I will eventually put it in a separate package, but I will make sure that PlutoTeachingTools does not break (it is API included in PlutoUI semver).

@fonsp
Copy link
Member

fonsp commented Oct 2, 2024

Ah sorry I did not look at the code properly:

Using PlutoRunner as a dependency will not work, but you can use PlutoUI.ExperimentalLayout.Div, which works the same.

@fonsp
Copy link
Member

fonsp commented Oct 2, 2024

About the change in layout: I highly recommend using PlutoUI.ExperimentalLayout.Div instead of the current solution with HTML, because it is much more general! It allows placing interactive elements inside layouts, and much more.

I'm sure that the difference in size with text boxes can be fixed using the new code, maybe you could share a screenshot to show the difference that you notice?

Having both methods in the same package might be quite confusing when users try to make these more complex layouts with interactive elements and such.

@greimel
Copy link
Collaborator Author

greimel commented Oct 2, 2024

thanks @fonsp

It works now as intended. (I had too many "display: flex")

image

example.jl Outdated Show resolved Hide resolved
@@ -1,5 +1,6 @@
module PlutoTeachingTools

using PlutoUI.ExperimentalLayout: Div
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason to use using here instead of import?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you should only use import if you extend a function.

(This consistent with the rest of the package.)

@greimel
Copy link
Collaborator Author

greimel commented Oct 4, 2024

@eford do you approve these changes?

If so, I'd merge and tag a new release so that I can use these changes for my course material.

(These changes are a strict improvement. The problem about width of boxes was solved.)

@eford eford merged commit c6facca into JuliaPluto:main Oct 4, 2024
3 checks passed
@eford
Copy link
Collaborator

eford commented Oct 4, 2024

Thanks. Merged from phone. I can do a new version later today.

@greimel
Copy link
Collaborator Author

greimel commented Oct 4, 2024

Thanks, I've registered a new version.

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.

Generalize show for TwoColumn etc to work with svgs
3 participants