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

JanhaviAlekar · 2024-03-06T15:07:59Z

Before :
Screencast from 05-03-24 03:47:55 PM IST.webm

After:
Screencast from 06-03-24 07:35:35 PM IST.webm

personalizedrefrigerator

Thank you for the pull request!

Based on the screen recording and this comment, it looks like there are still a few changes to be made — currently, the "toggle external editing" button has right padding when not external editing:

personalizedrefrigerator · 2024-03-06T15:20:51Z

packages/app-desktop/gui/NoteEditor/NoteBody/TinyMCE/styles/index.ts

@@ -38,7 +39,7 @@ export default function styles(props: NoteBodyEditorProps) {
 			},
 			leftExtraToolbarContainer: {
 				...extraToolbarContainer,
-				width: 80,
+				width: { leftExtraToolbarContainerWidth },


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.

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

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.

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.

personalizedrefrigerator · 2024-03-06T15:22:37Z

packages/app-desktop/gui/NoteEditor/NoteBody/TinyMCE/TinyMCE.tsx

+				padding-left: ${120 + styles.leftExtraToolbarContainer.padding * 2}px;
+				padding-right: ${120 + styles.rightExtraToolbarContainer.padding * 2}px;


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).

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

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

laurent22 · 2024-03-14T18:15:11Z

@personalizedrefrigerator, what would be the status of this pull request? (whenever reviewing a pull request, please mark it as "approved" once the changes are made and are satisfactory)

personalizedrefrigerator · 2024-03-14T19:25:28Z

@personalizedrefrigerator, what would be the status of this pull request? (whenever reviewing a pull request, please mark it as "approved" once the changes are made and are satisfactory)

Thank you for the reminder! I plan to test this pull request locally later today.

personalizedrefrigerator · 2024-03-14T20:53:52Z

packages/app-desktop/gui/NoteEditor/NoteBody/TinyMCE/TinyMCE.tsx

@@ -351,14 +351,15 @@ const TinyMCE = (props: NoteBodyEditorProps, ref: any) => {
 	useEffect(() => {
 		const theme = themeStyle(props.themeId);
 		const backgroundColor = props.whiteBackgroundNoteRendering ? lightTheme.backgroundColor : theme.backgroundColor;
+		const leftExtraToolbarContainerWidth = props.watchedNoteFiles.length > 0 ? 120 : 80;


Currently, this logic is present in both TinyMCE.tsx and index.ts. Please refactor this to avoid duplicate code.

Edit: The index.ts logic might not be used anymore (see above comment).

personalizedrefrigerator

Thank you for working on this!

I've tested the changes locally and left a few comments.

personalizedrefrigerator · 2024-03-14T21:06:52Z

packages/app-desktop/gui/NoteEditor/NoteBody/TinyMCE/TinyMCE.tsx

-				padding-left: ${styles.leftExtraToolbarContainer.width + styles.leftExtraToolbarContainer.padding * 2}px;
-				padding-right: ${styles.rightExtraToolbarContainer.width + styles.rightExtraToolbarContainer.padding * 2}px;
+				padding-left: ${leftExtraToolbarContainerWidth + styles.leftExtraToolbarContainer.padding * 2}px;
+				padding-right: ${styles.rightExtraToolbarContainerWidth + styles.rightExtraToolbarContainer.padding * 2}px;


Currently, padding-right is being set to NaNpx. When I run

console.log(document.querySelector('#tinyMceStyle').innerText.substring(0, 100))

from Joplin's debug terminal, I get

.joplin-tinymce .tox-editor-header { padding-left: 92px; padding-right: NaNpx; }

Please double-check that the property names are correct here!

Apologies, all corrected now.

personalizedrefrigerator · 2024-03-14T21:11:23Z

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

Please change the title to

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

We have a script that parses commit titles and converts them into release notes. When pull requests are sqashed and merged, the default commit title is the pull request title.

…laps-with-bold-button

personalizedrefrigerator · 2024-03-15T21:54:44Z

packages/app-desktop/gui/NoteEditor/NoteBody/TinyMCE/TinyMCE.tsx

@@ -547,7 +547,7 @@ const TinyMCE = (props: NoteBodyEditorProps, ref: any) => {
 		//
 		// tl;dr: editorReady is used here because the css needs to be re-applied after TinyMCE init
 		// eslint-disable-next-line @seiyab/react-hooks/exhaustive-deps -- Old code before rule was applied
-	}, [editorReady, props.themeId, lightTheme, props.whiteBackgroundNoteRendering]);
+	}, [editorReady, props.themeId, lightTheme, props.whiteBackgroundNoteRendering, props.watchedNoteFiles]);


Because props.watchedNoteFiles isn't used in the body of that hook, consider leaving a comment about why props.watchedNoteFiles is included in the dependency array.

Alternatively, it could make more sense to depend on styles instead of props.watchedNoteFiles.

Other than that, this pull request looks good to me!

I added Comment , additionally I want to know watchedNoteFiles is used for checking if external editor is on or off but if there is anything which keeps track of each notes state If that note is open in external editor?

is anything which keeps track of each notes state If that note is open in external editor?

ExternalEditWatcher.ts is responsible for watching for file updates. The note is updated here:

joplin/packages/lib/services/ExternalEditWatcher.ts

Lines 144 to 145 in 7638bdf

await Note.save(updatedNote);

this.eventEmitter_.emit('noteChange', { id: updatedNote.id, note: updatedNote });

The noteChange event is handled here:

joplin/packages/app-desktop/gui/NoteEditor/NoteEditor.tsx

Line 346 in 7638bdf

const externalEditWatcher_noteChange = useCallback((event: any) => {

What I think then happens is props.content is updated, which causes the rich text editor to update its content, near this comparison:

joplin/packages/app-desktop/gui/NoteEditor/NoteBody/TinyMCE/TinyMCE.tsx

Line 906 in 7638bdf

if (lastOnChangeEventInfo.current.content !== props.content || !resourcesEqual) {

Alternatively, it could make more sense to depend on styles instead of props.watchedNoteFiles.

I think that would be the better fix - styles is in use in this effect but not in the dependencies. This change may be a good opportunity to clean up this effect and remove the legacy eslint-disable-next-line @seiyab/react-hooks/exhaustive-deps (which allows this new change to pass the linter even though it shouldn't). It will require extensive testing though @JanhaviAlekar

@JanhaviAlekar please let me know if you're interested in completing this pull request. I assume that changing the dependency to styles is not too complex.

@JanhaviAlekar please let me know if you're interested in completing this pull request. I assume that changing the dependency to styles is not too complex.

Apologies for the delay. I was experiencing some laptop issues. I've made the required changes, and now only the testing part is left. I'll test it today.

@JanhaviAlekar please let me know if you're interested in completing this pull request. I assume that changing the dependency to styles is not too complex.

Apologies for the delay. I was experiencing some laptop issues. I've made the required changes, and now only the testing part is left. I'll test it today.

packages/app-desktop/gui/NoteEditor/NoteBody/TinyMCE/TinyMCE.tsx

[editorReady, props.themeId, lightTheme, props.whiteBackgroundNoteRendering, styles]);

I'd like to understand why lightTheme is included in the dependency array. When I removed the //eslint-disable-next-line @seiyab/react-hooks/exhaustive-deps , the linter raised an error indicating that lightTheme is a dependency not in use. I then removed it, and the code still worked fine. However, I'm uncertain about its purpose or necessity. Could you please provide clarification?

Ok then I don't know, we're probably going to break something somewhere if we remove it so let's leave and go back to the original idea. Just include props.watchedNoteFiles please and add a comment explaining why it's needed

Ok then I don't know, we're probably going to break something somewhere if we remove it so let's leave and go back to the original idea. Just include props.watchedNoteFiles please and add a comment explaining why it's needed

Done! Included props.watchedNoteFiles and added a comment explaining why it's needed. Thank you for your help!

…laps-with-bold-button

laurent22 · 2024-04-03T17:29:18Z

Looks good now, thanks you @JanhaviAlekar !

resolves toggle external editing button overlaps with bold button

76fb468

personalizedrefrigerator requested changes Mar 6, 2024

View reviewed changes

Toggle external editing container's padding fixed

62085ad

personalizedrefrigerator reviewed Mar 14, 2024

View reviewed changes

Merge branch 'laurent22:dev' into Toggle-external-editing-button-over…

361e6d0

…laps-with-bold-button

JanhaviAlekar changed the title ~~Desktop :Fixes #10025 Toggle external editing button overlaps with bold button.~~ Desktop: Fixes #10025: Toggle external editing button overlaps with bold button. Mar 15, 2024

Toggle external editing button overlaps with bold button

6d60843

JanhaviAlekar force-pushed the Toggle-external-editing-button-overlaps-with-bold-button branch from 4017832 to 6d60843 Compare March 15, 2024 18:50

JanhaviAlekar requested a review from personalizedrefrigerator March 15, 2024 19:02

personalizedrefrigerator reviewed Mar 15, 2024

View reviewed changes

toggle external editing button overlaps with bold button

d779ce5

personalizedrefrigerator approved these changes Mar 17, 2024

View reviewed changes

Merge branch 'laurent22:dev' into Toggle-external-editing-button-over…

c8112d7

…laps-with-bold-button

JanhaviAlekar force-pushed the Toggle-external-editing-button-overlaps-with-bold-button branch from 9a5b2f3 to c8112d7 Compare March 23, 2024 17:18

laurent22 merged commit ce3a28d into laurent22:dev Apr 3, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

JanhaviAlekar commented Mar 6, 2024

personalizedrefrigerator left a comment

personalizedrefrigerator Mar 6, 2024

JanhaviAlekar Mar 7, 2024

personalizedrefrigerator Mar 14, 2024

JanhaviAlekar Mar 15, 2024

personalizedrefrigerator Mar 6, 2024

JanhaviAlekar Mar 7, 2024

personalizedrefrigerator Mar 14, 2024

laurent22 commented Mar 14, 2024

personalizedrefrigerator commented Mar 14, 2024

personalizedrefrigerator Mar 14, 2024 •

edited

personalizedrefrigerator left a comment

personalizedrefrigerator Mar 14, 2024

JanhaviAlekar Mar 15, 2024

personalizedrefrigerator commented Mar 14, 2024

personalizedrefrigerator Mar 15, 2024 •

edited

JanhaviAlekar Mar 16, 2024

personalizedrefrigerator Mar 17, 2024

laurent22 Mar 17, 2024

laurent22 Mar 20, 2024

JanhaviAlekar Mar 21, 2024

JanhaviAlekar Mar 23, 2024

laurent22 Mar 23, 2024

JanhaviAlekar Mar 23, 2024

laurent22 commented Apr 3, 2024

	width: { leftExtraToolbarContainerWidth },
	width: leftExtraToolbarContainerWidth,

		padding-left: ${120 + styles.leftExtraToolbarContainer.padding * 2}px;
		padding-right: ${120 + styles.rightExtraToolbarContainer.padding * 2}px;

	await Note.save(updatedNote);
	this.eventEmitter_.emit('noteChange', { id: updatedNote.id, note: updatedNote });

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

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

Conversation

JanhaviAlekar commented Mar 6, 2024

personalizedrefrigerator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurent22 commented Mar 14, 2024

personalizedrefrigerator commented Mar 14, 2024

personalizedrefrigerator Mar 14, 2024 • edited

Choose a reason for hiding this comment

personalizedrefrigerator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

personalizedrefrigerator commented Mar 14, 2024

personalizedrefrigerator Mar 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurent22 commented Apr 3, 2024

personalizedrefrigerator Mar 14, 2024 •

edited

personalizedrefrigerator Mar 15, 2024 •

edited