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

Add setting for max image width when viewing .note files #24

Merged
merged 6 commits into from
Dec 19, 2024

Conversation

james-xli
Copy link
Contributor

@james-xli james-xli commented May 13, 2024

Per issue #22, I took a stab at adding a max px width setting in the settings pane:

Screenshot 2024-05-12 at 11 18 09 PM

Set to 500 px as shown above, my test note looks like this:

Screenshot 2024-05-12 at 11 18 36 PM

Notes are still responsive to window size below the max width.

The default value is 1400px, which is nearly the max width of a note (both A5X and A6X2) in portrait orientation. This is the same as the preexisting behavior. That same test note looks like this at the max 1400px width:

Screenshot 2024-05-12 at 11 18 45 PM

I am pretty new to contributing on Github so I would like any and all feedback, thanks!

@philips
Copy link
Owner

philips commented May 21, 2024

Hrm, I wonder how we would handle horizontal on this. My instinct is to make max-width a percentage at 5% increments or something instead. WDYT? Is there a reason you want pixel increment control?

@philips
Copy link
Owner

philips commented May 21, 2024

Note to self: I tested this branch and it works fine as described. Code looks fine. Just needs a squash after the percentage discussion above.

@james-xli
Copy link
Contributor Author

I think you're right, a percentage will probably be more intuitive for horizontal notes, as well as future larger notes from the A5X2 device. Pixels just happened to be convenient for implementation. I am going out of town for a week but will come back to this when I return!

@philips
Copy link
Owner

philips commented May 26, 2024 via email

james-xli added 4 commits June 8, 2024 17:20
Per issue philips#22, adds a max px width setting in the settings pane. Notes are still responsive to window size below the max width.
I think this makes the setting behavior more intuitive, because the slider value now always means what it seems to mean.

Previously, setting to 0px allowed the page to go to its full 1404px width, but that's basically the same as just setting 1400px for the max width.

I set the lower bound at 100px so that a user can't accidentally make the pages disappear entirely.

Note: I know pages in landscape orientation would have a width of 1872px, but I'm going with 1400px for now because:
- This plugin doesn't seem to be parsing landscape notes correctly at the moment
- 1400px is already very wide
I removed the special 0px behavior in the previous commit but forgot to update this text in the settings description.
@james-xli james-xli force-pushed the feature/note-max-width branch from 281d688 to 47084d5 Compare June 9, 2024 00:20
james-xli added 2 commits June 8, 2024 17:49
… instead of just width. Multiple pages can be visible horizontally.

I think this should play nice with horizontal pages, but am unable to test because horizontal pages aren't working quite yet.

Also changed the default setting to 800px, which is a nice medium size to be legible but not too big. Max setting now goes to 1900 px (slightly larger than the 1872 px long edge of a Nomad page).
@james-xli
Copy link
Contributor Author

james-xli commented Jun 9, 2024

I tried the percentage but couldn't find a clean way to implement my desired behavior. Doing something like max-width: 60% would cause the max width to vary based on the window size, whereas I wanted the max width constraint to stay constant regardless of window size.

(Maybe I'm totally misunderstanding what you were describing above, or overlooking something obvious? Please let me know if so!)

Instead, to handle horizontal pages nicely (i.e., keep them at the same apparent scale as vertical pages), I changed the setting so it limits both the height and width of the image. It's now called "Max image side length" in the Settings pane. For example, the new default limit of 800 px would allow a vertical page up to 800 px tall, or a horizontal page up to 800 px wide.

Unfortunately I couldn't actually test this on any actual horizontal pages since the library doesn't support those yet, but I'm hopeful that this will play nice when that time comes!

I implemented this by creating a new parent div with the max-width attribute for each page (and then the images have a max-height attribute). Creating a div for each page presented a convenient opportunity to add a related behavior I had been planning to try: The ability to show multiple pages horizontally if the window is large enough.

Some screenshots to show what I mean:

  1. When the window is narrow, the images are responsive to the window width:
    image

  2. If the window is made wider, at a certain point the image hits the maximum side length setting and stops getting bigger:
    image

  3. If you keep making the window wider, the image stays the same size:
    image

  4. If the window is made further wide enough, more than one page can be shown side by side:
    image

Please let me know what you think, thanks! Cheers :)

@james-xli
Copy link
Contributor Author

james-xli commented Jun 9, 2024

One more screenshot to demonstrate the added functionality: if I keep the window the same size as in the last screenshot above, but set the image side length limit to something smaller (here 400 px), now I can see a whole bunch of pages at once:

image

@ankit-kapur
Copy link

This looks great for being able to see pages side by side, can we merge this @philips

@philips philips merged commit 97caef4 into philips:main Dec 19, 2024
@philips
Copy link
Owner

philips commented Dec 19, 2024

Sorry for the long delay. I have made a beta release with this feature so I can test it for a bit on my own devices. You can install it with BRAT - it is 2.3.0

@james-xli
Copy link
Contributor Author

Thanks @philips! Glad to see you back here.

@philips
Copy link
Owner

philips commented Dec 19, 2024

First bug: At 200px on my iPhone the text contents have no padding so when two pages are adjacent the words run into each other.

@philips
Copy link
Owner

philips commented Dec 19, 2024

Filed issue #38

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.

3 participants