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.

Sign up for GitHub

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

Jump to bottom

Desktop: Fixes #10025: Toggle external editing button overlaps with bold button. #10069

Merged
merged 6 commits into from
Apr 3, 2024
Merged

Desktop: Fixes #10025: Toggle external editing button overlaps with bold button. #10069

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
76fb468
resolves toggle external editing button overlaps with bold button
JanhaviAlekar Mar 6, 2024
62085ad
Toggle external editing container's padding fixed
JanhaviAlekar Mar 7, 2024
361e6d0
Merge branch 'laurent22:dev' into Toggle-external-editing-button-over…
JanhaviAlekar Mar 15, 2024
6d60843
Toggle external editing button overlaps with bold button
JanhaviAlekar Mar 15, 2024
d779ce5
toggle external editing button overlaps with bold button
JanhaviAlekar Mar 16, 2024
c8112d7
Merge branch 'laurent22:dev' into Toggle-external-editing-button-over…
JanhaviAlekar Mar 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/app-desktop/gui/NoteEditor/NoteBody/TinyMCE/TinyMCE.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,8 +357,8 @@ const TinyMCE = (props: NoteBodyEditorProps, ref: any) => {
document.head.appendChild(element);
element.appendChild(document.createTextNode(`
.joplin-tinymce .tox-editor-header {
padding-left: ${styles.leftExtraToolbarContainer.width + styles.leftExtraToolbarContainer.padding * 2}px;
padding-right: ${styles.rightExtraToolbarContainer.width + styles.rightExtraToolbarContainer.padding * 2}px;
padding-left: ${120 + styles.leftExtraToolbarContainer.padding * 2}px;
padding-right: ${120 + styles.rightExtraToolbarContainer.padding * 2}px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Currently, this always increases the size reserved for the toolbar button. Based on comment, I think it would make more sense to change the size conditionally. (This might require adding a dependency to the useEffect).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screencast.from.07-03-24.12.47.12.PM.IST.webm

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for the updated video! (I had been looking at the video attached under "After" in the PR description :) )

}

.tox .tox-toolbar,
Expand Down
3 changes: 2 additions & 1 deletion packages/app-desktop/gui/NoteEditor/NoteBody/TinyMCE/styles/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { NoteBodyEditorProps } from '../../../utils/types';
const { buildStyle } = require('@joplin/lib/theme');

export default function styles(props: NoteBodyEditorProps) {
const leftExtraToolbarContainerWidth = props.watchedNoteFiles.length > 0 ? 120 : 80;
return buildStyle(['TinyMCE', props.style.width, props.style.height], props.themeId, (theme: any) => {
const extraToolbarContainer = {
boxSizing: 'content-box',
Expand Down Expand Up @@ -38,7 +39,7 @@ export default function styles(props: NoteBodyEditorProps) {
},
leftExtraToolbarContainer: {
...extraToolbarContainer,
width: 80,
width: { leftExtraToolbarContainerWidth },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
width: { leftExtraToolbarContainerWidth },
width: leftExtraToolbarContainerWidth,

Including the {s sets width to an object with content

{
    leftExtraToolbarContainerWidth: leftExtraToolbarContainerWidth,
}

Setting width directly to leftExtraToolbarContainerWidth may make more sense here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, when we directly update leftExtraToolbarContainerWidth (width: leftExtraToolbarContainerWidth), it's only setting the value once and not updating the width when watchedNoteFiles changes. I've tried logging the value of leftExtraToolbarContainerWidth, which shows correctly. However, the problem is that the width of the container is not updating during changes in watchedNoteFiles. I've tried to figure it out, and I believe the issue is that the styles are initially loaded and not updating during changes in watchedNoteFiles
So in packages/app-desktop/gui/NoteEditor/NoteBody/TinyMCE/TinyMCE.tsx we can do this which will load styles when watchedNoteFiles changes
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've tried to figure it out, and I believe the issue is that the styles are initially loaded and not updating during changes in watchedNoteFiles

It should be possible to fix this by updating the dependency array (cacheKey) passed to buildStyles. I think making this change in TinyMCE.tsx would also be fine, but making the change in styles/index.ts should keep the styles in closer to the same place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing that out.I wasn't aware of the (cacheKey) concept before, but after your suggestion, I made the necessary changes in the styles/index.ts.

left: 0,
},
rightExtraToolbarContainer: {
Expand Down
1 change: 1 addition & 0 deletions packages/app-desktop/gui/NoteEditor/NoteEditor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,7 @@ function NoteEditor(props: NoteEditorProps) {
// We need it to identify the context for which media is rendered.
// It is currently used to remember pdf scroll position for each attachments of each note uniquely.
noteId: props.noteId,
watchedNoteFiles: props.watchedNoteFiles,
};

let editor = null;
Expand Down
1 change: 1 addition & 0 deletions packages/app-desktop/gui/NoteEditor/utils/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ export interface NoteBodyEditorProps {
isSafeMode: boolean;
noteId: string;
useCustomPdfViewer: boolean;
watchedNoteFiles: string[];
}

export interface FormNote {
Expand Down