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

display: grid for editor toolbar, editor and preview #7787

Merged
merged 7 commits into from Oct 17, 2023

Conversation

BurningTreeC
Copy link
Contributor

This PR makes the div surrounding the toolbar, editor and preview display: grid
It also adds the necessary styles so that editor and preview are displayed full width, not 49% as before
It also takes into account that the bitmap editor has variable width and height

see Idea #7771

@vercel
Copy link

vercel bot commented Oct 15, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
tiddlywiki5 ✅ Ready (Inspect) Visit Preview Oct 17, 2023 3:22am

@BurningTreeC
Copy link
Contributor Author

Should there also be a fullscreen button for the editor?

@BurningTreeC
Copy link
Contributor Author

Hello @pmario - I created an alternative PR here: #7788

Copy link
Contributor

@pmario pmario left a comment

Choose a reason for hiding this comment

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

I think that's much better now.

themes/tiddlywiki/vanilla/base.tid Outdated Show resolved Hide resolved
themes/tiddlywiki/vanilla/base.tid Outdated Show resolved Hide resolved
themes/tiddlywiki/vanilla/base.tid Outdated Show resolved Hide resolved
themes/tiddlywiki/vanilla/base.tid Outdated Show resolved Hide resolved
@BurningTreeC
Copy link
Contributor Author

Hi @Jermolene @pmario - I think this is ready for review

@pmario
Copy link
Contributor

pmario commented Oct 16, 2023

I think, we also needs some documentation, that tells users about the new CSS tc-grid classes and the grid areas.
May be we add this to the https://tiddlywiki.com/dev/ edition.

It should make it easier for plugin authors to deal with the new grid areas

@pmario
Copy link
Contributor

pmario commented Oct 16, 2023

@Jermolene -- It looks good to me now

Copy link
Owner

@Jermolene Jermolene left a comment

Choose a reason for hiding this comment

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

@@ -18,7 +18,7 @@ $:/config/EditorToolbarButtons/Visibility/$(currentTiddler)$
importState=<<qualify $:/state/ImportImage>> >
<$dropzone importTitle=<<importTitle>> autoOpenOnImport="no" contentTypesFilter={{$:/config/Editor/ImportContentTypesFilter}} class="tc-dropzone-editor" enable={{{ [{$:/config/DragAndDrop/Enable}match[no]] :else[subfilter{$:/config/Editor/EnableImportFilter}then[yes]else[no]] }}} filesOnly="yes" actions=<<importFileActions>> >
<div>
<div class={{{ [function[edit-preview-state]match[yes]then[tc-tiddler-preview]] +[join[ ]] }}}>
<div class={{{ [function[edit-preview-state]match[yes]then[tc-tiddler-preview]else[tc-no-tiddler-preview]] [[tc-grid]] +[join[ ]] }}}>
Copy link
Owner

Choose a reason for hiding this comment

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

We try to use semantic class names, and avoid functional names such as "tc-grid". I think it would be clearer to replace tc-no-tiddler-preview with tc-tiddler-preview-hidden, and to use tc-tiddler-preview-frame instead of tc-grid.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think tc-tiddler-preview-frame is way to specific. tc-grid only contains 1 setting to use grid. It can be reused that way.

tc-tiddler-preview-frame is completely misleading since it is not set to the preview-frame, but it's parent

Copy link
Contributor

Choose a reason for hiding this comment

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

If I want to define tc-grid to any other gird with a completely different context in the sidebar or control-panel tc-tiddler-preview-frame would be misleading. IMO it should be as generic as possible

Copy link
Owner

Choose a reason for hiding this comment

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

@pmario if we use tc-grid and reuse it elsewhere then we'll be constrained that we can't change the styling of just the preview pane. As I said before, functional class names are an anti-pattern.

Copy link
Owner

Choose a reason for hiding this comment

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

We could use tc-tiddler-editor instead of tc-grid.

@pmario
Copy link
Contributor

pmario commented Oct 16, 2023

@pmario if we use tc-grid and reuse it elsewhere then we'll be constrained that we can't change the styling of just the preview pane.

That's not true. Have a closer look at the CSS code.

tc-grid activates display: grid only. It may be used for some global styling for grids but I assume that this one will never be changed again.

tc-tiddler-preview, tc-tiddler-no-preview and tc-tiddler-preview-preview and may be others are the classes that are responsible for the editor and preview definitions.

IMO tc-grid is a utility calss, which defines a major usage pattern. It is a functional class name because there is no other good name for "grid".

We could use tc-framework but that would be misleading. tc-framework could be grid or flexbox. But those 2 definitions are not interchangeable. So it's needed to stay with tc-grid or a tc-flex if you started with it. They are not compatible.

@Jermolene
Copy link
Owner

Hi @pmario our CSS classes are designed not just to accomplish a particular visual layout but to provide a surface area for customisations. If we use a single tc-grid class then it becomes impossible for others to customise the layout of the preview pane without also affecting the other places that use tc-grid.

@pmario
Copy link
Contributor

pmario commented Oct 16, 2023

@BurningTreeC ... I did have a closer look again.

Jeremy wrote:

We could use tc-tiddler-editor instead of tc-grid.

Let's go with tc-tiddler-editor.

We will have the tc-tiddler-editor as a container class, that defines display: grid and
we have now tc-tiddler-preview and tc-tiddler-no-preview, which are used to enable/disable the preview and toolbar "child" elements

Can you check the following again?

For me tc-tiddler-no-preview makes more sense than tc-no-tiddler-preview -- Just a thought

@BurningTreeC
Copy link
Contributor Author

Hi @pmario @Jermolene

I went with tc-tiddler-editor and tc-tiddler-preview-hidden

Should be good now 👍

@Jermolene
Copy link
Owner

Terrific, thank you @BurningTreeC

@Jermolene Jermolene merged commit c185e37 into Jermolene:master Oct 17, 2023
4 checks passed
@oeyoews
Copy link
Contributor

oeyoews commented Jan 8, 2024

This seems to conflict with the flexbox layout of cm6 (maybe), but cm5 does not have this situation. @Jermolene @BurningTreeC

Screenity.video.-.Jan.9.2024.mp4

@Jermolene
Copy link
Owner

Hi @oeyoews are you saying that the new CSS grid layout of the editor pane interferes with CodeMirror 6? It's hard to see how that could happen if CM6 adapts correctly to the size of its container.

@oeyoews
Copy link
Contributor

oeyoews commented Jan 8, 2024

The reason is really not easy to investigate, and I don’t have any clue. What is certain is that if the width of the element is specified, it will cause this problem, unless width: 100% is specified. @Jermolene

Screenity.video.-.Jan.9.2024.1.mp4

@Jermolene
Copy link
Owner

Thanks @oeyoews could it be fixed by introducing a wrapper element with width: 100%?

@oeyoews
Copy link
Contributor

oeyoews commented Jan 8, 2024

No, it doesn't work

@oeyoews
Copy link
Contributor

oeyoews commented Jan 8, 2024

image

After looking at the usage of grid-template, this should fix the problem, but what I don't understand is why just cm6 would cause this problem, I'm having a hard time finding what the difference is. It is roughly the style of the cm6 editor, which results in inconsistent widths on the left and right, so it is necessary to clearly specify that each width is equal. @Jermolene

.tc-tiddler-preview {
grid-template-columns: repeat(2, minmax(0px, 1fr)) !important;
}

@oeyoews
Copy link
Contributor

oeyoews commented Jan 8, 2024

#7922

@pmario
Copy link
Contributor

pmario commented Jan 9, 2024

no !important is toxic. It will mess up all our stylesheets.

Jermolene pushed a commit that referenced this pull request Jan 24, 2024
@BurningTreeC BurningTreeC deleted the patch-60 branch March 29, 2024 15:35
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

4 participants