laurent22 / joplin

Joplin - the privacy-focused note taking app with sync capabilities for Windows, macOS, Linux, Android and iOS.
https://joplinapp.org
Other
45.19k stars 4.92k forks source link

ordered list digits cut off #4715

Open jgreely opened 3 years ago

jgreely commented 3 years ago

The digits in ordered lists extend into the margin and get cut off. The problem is much more severe on the iOS client, especially with wider fonts, so that "monospace" won't even show a single-digit number. On the Mac desktop client, it will at least display correctly until you hit 100 items in the list.

Environment

Joplin version: 10.7.2 Platform: iOS (iPhone XS & iPad Mini) OS specifics: 14.4.1

Steps to reproduce

  1. Create note or todo containing ordered list with 10 or more rows.
  2. On iOS, for items 10 and up, the first digit is cut off.
  3. Add a DIV tag at the top with style="padding-left:16px" to make the tens digit visible.
  4. Add additional items to the list to take it above 100 rows, and the leading digit is again cut off.
  5. Increase the padding to 32px to make the hundreds digit visible.

Describe what you expected to happen

All digits should be visible regardless of font or length of list.

ol-default-font ol-monospace-font

PIG208 commented 3 years ago

It appears that the same problem occurs to the desktop application on Windows as well when the size of the numbered list goes to 1000 or above. image image

CalebJohn commented 3 years ago

I just tested this on v1.7.11 Ubuntun 20.04 and v1.7.5 Android 11.

I could duplicate this behaviour in the viewer on android and on the WYSIWYG editor on the desktop. I could not duplicate this behaviour on the markdown editor/viewer on desktop.

tessus commented 3 years ago

I was able to reproduce the problem on

Joplin 1.8.0 (dev, darwin)

Client ID: e354a932e6e143c782264aad4112b674
Sync Version: 2
Profile Version: 34
Keychain Supported: Yes

Revision: 5b65186b4 (dev)

Although not at 100, but at 1,000.

image
jalajcodes commented 3 years ago

I can see this bug on Windows 10,

after 999 items in markdown editor/viewer. image

after 99 items in WYSIWYG editor image

Joplin 1.7.11 (prod, win32)

Client ID: a0c954327e82488cb38da36c5c2a167d
Sync Version: 2
Profile Version: 34
Keychain Supported: Yes

Revision: f560563d8 (master)
singhakshita commented 3 years ago

In noteStyle.ts , `margin-left : 1.7em ' is fixed , changing that to 2em fixes the issue ,but i guess making this dynamic will be a better choice . can i work on this issue

CalebJohn commented 3 years ago

@singhakshita Sure, go ahead. Be sure to read the guidelines first.

coderrsid commented 3 years ago

Is this solved? May i work on this?

singhakshita commented 3 years ago

if we create a list after 99 th element i.e 100 100 102 th element in the rich text editor and then switch to markdown and back to rich text editor it appears like this

Screenshot 2021-03-25 at 6 01 21 PM

it actually doesn't add space , all the 'ul' elements appears inside the 99 th elements ,it can be seen here

Screenshot 2021-03-25 at 6 03 39 PM

This issue has been seen on mac OS ,i am not sure if this persists on any other OS. I am not sure if this issue has to opened separately or should i send the pull request sorting out this issue as well in the same PR only(since it can be OS specific)

coderrsid commented 3 years ago

Well @singhakshita, i can reproduce this currently too on macOS. Although what i figured out was that it takes all the elements in a single list item after 99th element, that's why it doesn't add space.

singhakshita commented 3 years ago

yeah i said that only , i was just clarifying that it might look like its adding space but actually its adding list items inside that 99th element

coderrsid commented 3 years ago

Although @tessus , if i try to set a left margin to the ordered list to fix this, should i consider list upto 1000 or even greater? Thanks.

tessus commented 3 years ago

I'm not really sure, but IMO this should be dynamic. I'm not a UI designer, but from a coding standpoint a dynamic solution would be preferable. In my case it only happens afer the 99th item and I never have numbered lists that long.

jgreely commented 3 years ago

On a desktop Chrome-based browser on a Mac, I can reproduce the 1000-length list problem with straight HTML, so I think correctly rendering three-digit list numbers on all platforms is sufficient. Experimentally, I've found that adding ol li {padding-left:1.5rem} does the trick on Mac/iPhone/iPad, even with large monospace fonts.

stale[bot] commented 3 years ago

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may comment on the issue and I will leave it open. Thank you for your contributions.

PIG208 commented 3 years ago

ghostwriter uses list-style-position: inside; image

ol, ul {
    list-style-position: inside;
    padding: 0;
}

This can be the correct solution for us if we would like to change the way list items align if we want to support as many digits as we want.

Github uses a fixed padding of 2rem: image

.markdown-body ol, .markdown-body ul {
    padding-left: 2em;
}

Apparently, this runs into the same problem when it comes to 1000, but generally, this is fine because they keep the markdown in a container with a padding of 32px. image

I think the Github solution would be totally feasible in our case. By simply selecting an appropriate margin/padding we can support up to 99999 list items. This should cover most of the use cases (plus it might be noticeably unresponsive when a list reaches that many items, which is a more critical problem than the cut off). A dynamic solution might not be necessary.

harsh0032 commented 3 years ago

May i work on this ?

CalebJohn commented 3 years ago

@harsh0032 go ahead.

Ritesh-Aggarwal commented 3 years ago

Is this issue solved?

roman-r-m commented 3 years ago

Is this issue solved?

Doesn't seem so. You can try the latest pre-release and see for yourself.

Ritesh-Aggarwal commented 3 years ago

Is this issue solved?

Doesn't seem so. You can try the latest pre-release and see for yourself.

I checked and the issue still persists. Can i work on this?

PIG208 commented 3 years ago

@Ritesh-Aggarwal you can go ahead!

harsh0032 commented 3 years ago

Hi i am not working on this anymore

On Mon, Aug 2, 2021 at 1:28 PM PIG208 @.***> wrote:

@Ritesh-Aggarwal https://github.com/Ritesh-Aggarwal you can go ahead!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/laurent22/joplin/issues/4715#issuecomment-890808936, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKM72D6SORQR7WAEXBNBK6DT2ZF3VANCNFSM4ZQZUW4Q .

Ritesh-Aggarwal commented 3 years ago

I am trying to setup for development but i am getting errors when i run npm install in root on windows I checked the issue and found this forum but it isn't still working.Can anyone refer to a better solution to this issue. forum mentioned: https://discourse.joplinapp.org/t/error-while-setting-up/15157/6

JackGruber commented 3 years ago

@Ritesh-Aggarwal please create a post on discourse for your installation problem of the dev env.

ghost commented 2 years ago

Hi. I just confirmed the issue yesterday at my Android and IOS devices. I could not go over 100. Is there any work being done on this issue?

BilalAtique commented 2 years ago

Hi, I have examined the issue in both android and window 10. In android, the problem occurs for 3 digit numbers ( after 99 ): joplin android bug In window 10, the problem occurs for 4 digit numbers ( after 999 ): jolpin desktop bug What I want to ask is Can I work on it?

CalebJohn commented 2 years ago

@MuhammadBilalBinAtique go ahead, but please check the previous attempts to fix this and make sure your solution is better. This is a complex problem and only correct solutions will be accepted.

BilalAtique commented 2 years ago

@CalebJohn Can you please share some views on what the correct solution will look like?

CalebJohn commented 2 years ago

@CalebJohn Can you please share some views on what the correct solution will look like?

Sorry @MuhammadBilalBinAtique, I have not looked into this problem deeply and cannot guide you in solving it.

tessus commented 2 years ago

@MuhammadBilalBinAtique please do not post text as screenshots. seriously how do people come up with idiotic idea?

And then please maybe search for the problem on the forum or ask there. gh is used to discuss reproducible errors. The problem is known, nothing left to discuss. So either you can come up with a PR or use the forum for questions.

jooji-san commented 2 years ago

observations by @PIG208 are very helpful. We can use this css property to avoid hard-coding margins:

ol {
    list-style-position: inside;
}

In #4821 @laurent22 was interested in what other markdown editors did. I checked GitHub and VSCode and turns out they also can't handle big numbers elegantly. They largely avoid this issue as @PIG208 already stated by having a padding.

jooji-san commented 2 years ago

The snippet below renders in browser (Firefox) by default like this (the same applies for Joplin):

<div>
  here's some text
  <ol>
    <li>item 1</li>
    <li>item 2</li>
  </ol>
  <ul>
    <li>item 1</li>
    <li>item 2</li>
  </ul>
</div>

image

If we have large numbers: image

If we use above mentioned property:

ol {
    list-style-position: inside;
}

image

However, as you can see there is a mismatch in indentation between ordered and unordered lists. To remedy this we can either apply the same list-style-position property to ul also and set both ul and ol padding to 0: image Or we could just set the ol padding so that it matches the indentation of the ul. 25px worked nicely in the browser for instance (ul padding is also set in pixels by default).: image I prefer the second way as we don't alter the styling of the unordered list as it is not subject of this issue.

jooji-san commented 2 years ago

I will start working on the PR when we will decide on which way to go.

niharikamahajan02 commented 2 years ago

Is this issue still open? Anyone working on it?

jooji-san commented 2 years ago

I am waiting for a response from the contributors.

CalebJohn commented 2 years ago

@jooji-san I think it doesn't hurt to change the styling for ul along with ol, as long as they have the same behaviour.

I don't know if you've deeply gone into the code yet, but there is one place where the styling for both is applied together, I think it makes sense to apply this change there. You can change the list-style-position to inside, and then adjust the margin all from the same place.

That seems to be the correct fix, but it does suffer from a change in the visuals. Personally I think it will be okay, but I'm certain that some users won't be happy with it. Perhaps this is something that can be put to a vote on the forum?

Before

image

After

image

(notice how it's not the first character in the numbers that is aligned, rather than the last)

jooji-san commented 2 years ago

yes, the fix is very easy to implement, but it may annoy people, so voting is fair.

ankit-jha1210 commented 2 years ago

Is this issue still open

SayanDeveloper commented 2 years ago

Sir, what if I will apply a check using javascript that how many elements are there? on the basis of that style can be modified. Though this might create a little bit of time complexity.

EshanSingh-ES commented 2 years ago

Is this a UIKit app? If yes where can I find the Xcode project for this to reproduce this issue ? I am a beginner and have never contributed before

VitoPareto commented 2 years ago

Is someone currently working on this issue?

namannangia commented 1 year ago

Can i work on it ?

roman-r-m commented 1 year ago

Can i work on it ?

Sure

CalebJohn commented 1 year ago

@Ankitvnsbihar somebody else asked to work on it 2 days ago (see above). Please wait for them to finish or hand off the task.

tessus commented 1 year ago

We have explained many times that we do not assign issues. One of the reasons is that almost never a progress is made. A person asks if they can work on X and even if there's a reply that says "yes", you get a reply "thanks" but that was it. Weeks later.... No PR. Nothing. Nada. Zilch.

Now imagine this was assigned. It just doesn't make sense. Assignment only makes sense for core project members. And even then we rarely do that.

Thus, if you want to work on somehing, work on it. Create a valid PR that includes test cases for non GUI code and a manual testing process log for UI related code.

P.S.: Just look how man people asked if they could work on this. As you can see, the issue is still open, which means NOBODY actually did create a valid and correct PR.

github-actions[bot] commented 1 year ago

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? If you require support or are requesting an enhancement or feature then please create a topic on the Joplin forum. This issue may be closed if no further activity occurs. You may comment on the issue and I will leave it open. Thank you for your contributions.

Daeraxa commented 1 year ago

@laurent22 I've added a medium label just to stop the stale bot, feel free to change it if you need

laurent22 commented 1 year ago

Good call, we still would like that to be fixed some day but it wasn't a good GFI

NEK-RA commented 1 year ago

UPD: for me in version 3.0.8 for android ordered list is displayed properly now

~~Facing something similar, but in android version - numbers in range 0-99 works fine in both editor and view mode. numbers 100+ are okay while in editor mode, but in view mode it became cut off, so 100 turning into 00 and so on screenshot~~