Mobile, Desktop: Make table horizontally scrollable by give table a div parent, so the width of the table is limited to the screen width. #10161

wljince007 · 2024-03-20T05:49:33Z

Summary

Table adds div parent, so the width of the table is limited to the screen width. #8994 use jQuery or handy-scroll to implement scrollbar floating on Desktop end, introducing too much code, this PR does not dependent on jQuery or handy-scroll, do not modify table layout logic on desktop.

Mobile Screen

android test:

android_test_small.mp4

Ios test:

ios_test_small.mp4

desktop test:

desktop_test_small.mp4

Implementation Description

Add a markdown option in packages/lib/models/Setting.ts:

'markdown.plugin.tableHorizontallyScrollable': { storage: SettingStorage.File, isGlobal: true, value: false, type: SettingItemType.Bool, section: 'markdownPlugins', public: true, appTypes: [AppType.Mobile, AppType.Desktop], label: () => `${_('Enable table horizontally scrollable')}${wysiwygNo}` },

packages/renderer/MdToHtml/rules/tableHorizontallyScrollable.ts: make table adds div parent, make table horizontally scrollable.

<div class="joplin-table-div" >
      <table class="maps-to-line" source-line="186" source-line-end="312">
     .....
      </table>
</div>

…le is limited to the screen width on mobile devices

personalizedrefrigerator

Thank you for working on this!

I've left a few comments below.

personalizedrefrigerator · 2024-03-20T21:56:16Z

packages/app-mobile/utils/shim-init-react.js

@@ -249,6 +250,10 @@ function shimInit() {
 		});
 	};

+	platformUtil.mobilePlatform = () => {


Be aware that shim-init-react.js isn't run within the mobile WebView where rendering happens. As such, platform.mobilePlatform() may return '' while rendering.

personalizedrefrigerator · 2024-03-20T22:01:05Z

packages/renderer/noteStyle.ts

+		}
+
+		.joplin-table-div{
+			overflow-x: overlay;


Suggested change

overflow-x: overlay;

overflow-x: auto;

It looks like overflow: overlay; is a legacy alias for overflow: auto — consider using overflow-x: auto here instead.

personalizedrefrigerator · 2024-03-20T22:05:04Z

packages/renderer/noteStyle.ts

@@ -58,6 +59,9 @@ export default function(theme: any, options: Options = null) {

 	const fontFamily = '\'Avenir\', \'Arial\', sans-serif';

+	const isMobile = platformUtil.isMobile();


Consider adding an option to Options rather than moving platformUtil to utils. Because rendering now happens within a WebView (see e92f89d), after merging with dev, platformUtil.isMobile will return false in the renderer on mobile platforms.

personalizedrefrigerator · 2024-03-20T22:06:33Z

packages/renderer/noteStyle.ts

 		.mce-content-body .joplin-editable {
 			cursor: pointer !important;
+			${overflowHidden}
 		}


I think .mce-content-body should apply to the TinyMCE rich text editor. Consider removing this or adding a comment explaining why this style is applied here.

personalizedrefrigerator · 2024-03-20T22:07:29Z

packages/renderer/MdToHtml/rules/table_open.ts

+		}
+		const cur = String(self.renderToken(tokens, idx, options));
+		logger.info(`cur:${cur}`);
+		return platformUtil.isMobile() ? `<div class="joplin-table-div"  data-handy-scroll>\n${cur}` : cur;


Suggested change

return platformUtil.isMobile() ? `<div class="joplin-table-div" data-handy-scroll>\n${cur}` : cur;

return platformUtil.isMobile() ? `<div class="joplin-table-div" >\n${cur}` : cur;

personalizedrefrigerator · 2024-03-20T22:08:10Z

packages/renderer/MdToHtml/rules/table_open.ts

+			tokens[idx].attrSet('source-line-end', `${lineEnd}`);
+		}
+		const cur = String(self.renderToken(tokens, idx, options));
+		logger.info(`cur:${cur}`);


Consider removing this and the other similar logger.info statement.

wljince007 · 2024-03-21T01:16:59Z

@personalizedrefrigerator Thank you for patient guidance. I will try to add an option to control whether the table is restricted to div; I did not fully understand the design and multiple running environments of Joplin, and I will no longer modify shim.ts .

overflow: overlay; is a legacy alias for overflow: auto Co-authored-by: Henry Heino <46334387+personalizedrefrigerator@users.noreply.github.com>

Remove joplin-table-div incorrect CSS attribute Co-authored-by: Henry Heino <46334387+personalizedrefrigerator@users.noreply.github.com>

…le is limited to the screen width on mobile devices, add an option to control whether the table is restricted to div

laurent22 · 2024-03-21T10:58:07Z

packages/renderer/MdToHtml.ts

+	table_open: require('./MdToHtml/rules/table_open').default,
+	table_close: require('./MdToHtml/rules/table_close').default,


Since these two functions work together, could you put them together in the same file and use a meaningful name please? The basic structure would be like this:

export default { plugin: (markdownIt: any, params: any) => { markdownIt.renderer.rules.table_close = function(tokens: any[], idx: number, options: any, _env: any, self: any) { // ... }; markdownIt.renderer.rules.table_open = function(tokens: any[], idx: number, options: any, _env: any, self: any) { // ... }; }, };

What does the plugin do exactly? It makes the table horizontally scrollable on mobile, is that correct?

Yes, this plugin makes the table horizontally scrollable on mobile, so the maximum width of the table is the screen width. When dragging notes down, the interface does not sway left or right.

<div class="joplin-table-div" > <table class="maps-to-line" source-line="186" source-line-end="312"> ..... </table> </div>

@laurent22 I didn't have a deep understanding of Joplin before, and under your guidance, I have re implemented it by adding options. And I found that makes the table horizontally scrollable on desktop platforms, operating experience is also good.

export default { plugin: (markdownIt: any, params: any) => { markdownIt.renderer.rules.table_close = function(tokens: any[], idx: number, options: any, _env: any, self: any) { // ... }; markdownIt.renderer.rules.table_open = function(tokens: any[], idx: number, options: any, _env: any, self: any) { // ... }; }, };

Rebuild to: packages/renderer/MdToHtml/rules/tableHorizontallyScrollable.ts

Do you agree with the option name "tableHorizontalyScrollable"? If you agree, I will try to commit modifications of
introduction options(readme/apps/markdown.md) and translations file(packages/tools/locales)

…zontally scrollable

laurent22 · 2024-03-22T18:08:36Z

Thanks for taking the time to implement this @wljince007. Just two things:

Is it possible to add tests for it? Have a look at packages/app-cli/tests/MdToHtml.ts, I think you can just add a test case for your change.
Why is there this option markdown.plugin.tableHorizontallyScrollable? I'm thinking everybody would benefit from this change, so there's no need to make it optional?

wljince007 · 2024-03-23T01:32:57Z

@laurent22
Added test: packages/app-cli/tests/md_to_html/{table_horizontally_scrollable_1.md, table_horizontally_scrollable_2.md}, test runs ok.

At that time, I wanted to distinguish the use of floating scrollbars when loading plugins on the desktop, but now it seems unnecessary to add an option, already remove this option.

…crollable setting option

laurent22 · 2024-03-23T14:12:14Z

packages/renderer/MdToHtml/rules/tableHorizontallyScrollable.ts

+			if (tokens[idx].map) {
+				const line = tokens[idx].map[0];
+				const lineEnd = tokens[idx].map[1];
+				tokens[idx].attrJoin('class', 'maps-to-line');
+				tokens[idx].attrSet('source-line', `${line}`);
+				tokens[idx].attrSet('source-line-end', `${lineEnd}`);
+			}


Why is this necessary?

copy from packages/renderer/MdToHtml/rules/source_map.ts, I will remove it.

laurent22 · 2024-03-23T14:13:55Z

packages/renderer/MdToHtml/rules/tableHorizontallyScrollable.ts

@@ -0,0 +1,19 @@
+export default {
+	plugin: (markdownIt: any) => {
+		markdownIt.renderer.rules.table_open = function(tokens: any[], idx: number, options: any, _env: any, self: any) {


Please don't use any for any of the variables that you use. So tokens, options and self should have a type. You can get it from the markdown-it package

this code is copy from packages/renderer/MdToHtml/rules/source_map.ts.
The exact type of self variable is Renderer, but type Renderer has not been exported by markdown-it.

change to

markdownIt.renderer.rules.table_open = function(tokens: MarkdownIt.Token[], idx: number, options: MarkdownIt.Options, _env: any, renderer: any) {

@laurent22 Need I refactoring packages/renderer/MdToHtml/rules/{checkbox.ts, code_inline.ts, fence.ts, fountain.ts, highlight_keywords.ts, html_image.ts, image.ts, katex.ts, link_close.ts, link_open.ts, mermaid.ts, sanitize_html.ts, source_map.ts} to add tokens, options types?

laurent22 · 2024-03-23T14:14:46Z

packages/renderer/MdToHtml/rules/tableHorizontallyScrollable.ts

+				tokens[idx].attrSet('source-line', `${line}`);
+				tokens[idx].attrSet('source-line-end', `${lineEnd}`);
+			}
+			const cur = String(self.renderToken(tokens, idx, options));


What is the return type of self.renderToken() - isn't it already a string?

Also please avoid abbreviations, so cur => current

Yes, redundant operations will be removed.

laurent22 · 2024-03-23T14:14:57Z

packages/renderer/MdToHtml/rules/tableHorizontallyScrollable.ts

+		markdownIt.renderer.rules.table_close = function(tokens: any[], idx: number, options: any, _env: any, self: any) {
+			const cur = String(self.renderToken(tokens, idx, options));
+			return `${cur}</div>\n`;
+		};


Same comments here

laurent22 · 2024-03-23T14:15:26Z

packages/renderer/noteStyle.ts

@@ -434,6 +434,11 @@ export default function(theme: any, options: Options = null) {
 			overflow-x: auto;
 		}

+		.joplin-table-div{


Name it joplin-table-wrapper please

laurent22 · 2024-03-25T11:27:30Z

It looks good now, thank you @wljince007!

Table adds div parent tags on mobile devices, so the width of the tab…

3a31333

…le is limited to the screen width on mobile devices

personalizedrefrigerator requested changes Mar 20, 2024

View reviewed changes

wljince007 and others added 4 commits March 21, 2024 09:34

Update packages/renderer/noteStyle.ts

8785a86

overflow: overlay; is a legacy alias for overflow: auto Co-authored-by: Henry Heino <46334387+personalizedrefrigerator@users.noreply.github.com>

Update packages/renderer/MdToHtml/rules/table_open.ts

77e59a2

Remove joplin-table-div incorrect CSS attribute Co-authored-by: Henry Heino <46334387+personalizedrefrigerator@users.noreply.github.com>

Table adds div parent tags on mobile devices, so the width of the tab…

f90b899

…le is limited to the screen width on mobile devices, add an option to control whether the table is restricted to div

delte joplin/utils/platformUtil

8900dae

laurent22 reviewed Mar 21, 2024

View reviewed changes

markdown.plugin.tableHorizontallyScrollable option control table hori…

3c99966

…zontally scrollable

This comment was marked as duplicate.

Sign in to view

wljince007 changed the title ~~Mobile: Table adds div parent on mobile, so the width of the table is limited to the screen width on mobile~~ Mobile, Desktop: Make table horizontally scrollable by give table a div parent, so the width of the table is limited to the screen width. Mar 21, 2024

add tableHorizontallyScrollable test units, remove tableHorizontallyS…

cba5cb7

…crollable setting option

laurent22 reviewed Mar 23, 2024

View reviewed changes

rebuild, add type, add comments

22908b4

laurent22 merged commit 2de5c1b into laurent22:dev Mar 25, 2024
10 checks passed

wljince007 deleted the table_add_div_on_mobile_only branch March 27, 2024 02:53

wljince007 restored the table_add_div_on_mobile_only branch March 27, 2024 03:01

		@@ -58,6 +59,9 @@ export default function(theme: any, options: Options = null) {

		const fontFamily = '\'Avenir\', \'Arial\', sans-serif';

		const isMobile = platformUtil.isMobile();

	return platformUtil.isMobile() ? `<div class="joplin-table-div" data-handy-scroll>\n${cur}` : cur;
	return platformUtil.isMobile() ? `<div class="joplin-table-div" >\n${cur}` : cur;

		table_open: require('./MdToHtml/rules/table_open').default,
		table_close: require('./MdToHtml/rules/table_close').default,

Mobile, Desktop: Make table horizontally scrollable by give table a div parent, so the width of the table is limited to the screen width. #10161

Mobile, Desktop: Make table horizontally scrollable by give table a div parent, so the width of the table is limited to the screen width. #10161

Conversation

wljince007 commented Mar 20, 2024 • edited

Summary

Mobile Screen

android test:

Ios test:

desktop test:

Implementation Description

Add a markdown option in packages/lib/models/Setting.ts:

packages/renderer/MdToHtml/rules/tableHorizontallyScrollable.ts: make table adds div parent, make table horizontally scrollable.

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

personalizedrefrigerator Mar 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

personalizedrefrigerator Mar 20, 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

wljince007 commented Mar 21, 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

This comment was marked as duplicate.

laurent22 commented Mar 22, 2024

wljince007 commented Mar 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wljince007 Mar 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wljince007 Mar 24, 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

laurent22 commented Mar 25, 2024

wljince007 commented Mar 20, 2024 •

edited

personalizedrefrigerator Mar 20, 2024 •

edited

personalizedrefrigerator Mar 20, 2024 •

edited

wljince007 commented Mar 21, 2024 •

edited

wljince007 commented Mar 23, 2024 •

edited

wljince007 Mar 24, 2024 •

edited

wljince007 Mar 24, 2024 •

edited