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

Support Box overflow and scrolling #432

Closed
gnidan opened this issue Apr 16, 2021 · 26 comments
Closed

Support Box overflow and scrolling #432

gnidan opened this issue Apr 16, 2021 · 26 comments

Comments

@gnidan
Copy link

gnidan commented Apr 16, 2021

Firstly: this library is amazing; I'm so excited to use it everywhere I possibly can. Sadly, this issue describes a use case that's blocking me from moving forward :(

I'm opening this issue and offering a corresponding Gitcoin bounty in hopes it will help. Thank you to anyone who can help make this happen!

Summary

This feature request seeks to cover the use case where a component may exceed the bounds of its container. Specifically, this issue seeks the following behaviors:

  • Allow <Box overflow={...}>, perhaps as implemented by Add overflow prop to Box component #393
  • Allow Box components to scroll their contents. Based on my research, Add overflow prop to Box component #393 alone is insufficient, since it doesn't provide a way for a component to measure what its full width/height would be. (When overflow="hidden" is provided, width/height are measured based on the final component on-screen, and there seems to be no way around this)

Related

PR #393 (Add overflow prop to Box component) is mostly complete for my concerns here. I outlined my concerns with scrolling / measurements in this comment, but no idea what the right answer is.

vadimdemedes/goodness-squad#5 describes a related use case (Fullscreen UIs). This issue doesn't target fullscreen UIs specifically; overflow/scrolling behavior should work regardless of whether the component is going to be fullscreen or not.

Acceptance criteria

See comment below

Notes

Please let me know if the requirements could use clarification! I'm very keen to get this working 🙇

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


This issue now has a funding of 1.0 ETH (2414.48 USD @ $2414.48/ETH) attached to it.

@chrisirhc
Copy link

Allow Box components to scroll their contents.
Do you mean the box will scroll the contents over time? Such as an animation?

Based on what you're saying sounds like you might need to:

  1. Render the inner box contents then measure it's height and width
  2. Now set the height and width explicitly and render it within the Box and scroll it.

This is assuming you don't know the inner box contents from the beginning, since it could be rendered via flexbox rules.

@gitcoinbot
Copy link

gitcoinbot commented Apr 17, 2021

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


Work has been started.

These users each claimed they can complete the work by 265 years, 7 months from now.
Please review their action plans below:

1) chrisirhc has applied to start work (Funders only: approve worker | reject worker).

I think I have a solution that involves:

  1. Render the inner box contents then measure it's height and width
  2. Now set the height and width explicitly and render it within the Box and scroll it.
    2) developerfred has been approved to start work.

I would love to work on this issue, I have experience in reacting and I have already done something similar in a lib of angular components, it will be a very interesting challenge to conclude.
3) davidbanu has applied to start work (Funders only: approve worker | reject worker).

I would love to work on this! Would solve the two issues, where a component may exceed the bounds of its container, in a timeframe of 5-7 days max.
4) l2ichshop2 has applied to start work (Funders only: approve worker | reject worker).

ขอความจริง และความชัดเจนในการทำงาน ที่ถูกต้อง ไม่อ้อมค้อม ทั้งภารกิจ วิธีการ และกำหนดเวลา ขอบคุณครับ

Learn more on the Gitcoin Issue Details page.

@chrisirhc
Copy link

@gnidan
Copy link
Author

gnidan commented Apr 18, 2021

@chrisirhc I don't see where your solution measures the full size of the inner contents... it seems like this only scrolls a known amount. I really want to be able to implement an up/down pager (think less), but the problem I run into is that there's no way of knowing when we've scrolled to the bottom. Here's what I'm doing:

import React from "react";
import { Box, Text } from "ink";

import { loremIpsum } from "lorem-ipsum";

export const Paragraphs = ({ count = 10 }) => {
  const raw = loremIpsum({
    units: "paragraphs",
    count
  });

  const text = raw.split("\n").join("\n\n");

  return (
    <Box>
      <Text>{text}</Text>
    </Box>
  )
}

export const Main = () => {
  return (
    <Box width={80} height={25}>
      <AmazeBoxRenderer>
        <Box width={80}>
          <Paragraphs count={10} />
        </Box>
      </AmazeBoxRenderer>
    </Box>
  );
}

When I run this, I see 80 25 logged, which seems to suggest that the renderer believes the full height of the content to be merely 25 lines. This isn't the case, since ten paragraphs of lorem ipsum exceeds that at 80 characters. Does this break your assumptions here?

To get to your other points,

Based on what you're saying sounds like you might need to:

  1. Render the inner box contents then measure it's height and width
  2. Now set the height and width explicitly and render it within the Box and scroll it.

I'm not sure this works, since Ink's renderer will respect the outer wrapper's bounds rather than letting some inner content render with whatever natural dimensions it has.

This is assuming you don't know the inner box contents from the beginning, since it could be rendered via flexbox rules.

Definitely want to maintain this assumption.

I'll write another comment to follow-up with some acceptance criteria, since I've thought of it. Regardless, thanks for taking an initial look @chrisirhc! Tricky problem, right?

@gnidan
Copy link
Author

gnidan commented Apr 18, 2021

Acceptance criteria

  • A1. Given component A of unknown "natural size" (xA, yA) inside parent component B of fixed size (xB, yB),

    • A1.1. A must render within the bounds of B with overflow hidden

    • A1.2. Either A or B must be able to compute A's natural size (xA, yA), e.g. to ensure that A never completely leaves the viewport of B.

  • A2. Solutions must perform reasonably well without noticeable re-rendering or other flickering. This might mean fixing the flickering described in what to do when a view gets longer than the screen as it flickers badly on updates #359, but not necessarily (since A1 paired with stdout measurement means it'd be trivial to prevent rendering something larger than the terminal).

  • A3. Solutions must work with existing components in the ecosystem, including ink-spinner, ink-select-input, ink-progress-bar, etc. This means one of two options:

    • A3 (option 1, strongly preferred). Solution is merged and released as part of this library's official distribution channel. This means getting @vadimdemedes's approval, but I don't want to rely on that for this bounty.

    • A3 (option 2). Solution is releasable as a forked NPM package. Two stipulations:

      • A3(2).1. Changes are opened as PR here (so that I may seek to champion getting the changes into a main-line release once your work is done)

      • A3(2).2. Solution includes clear instructions on getting the above-mentioned component packages to work cleanly. Those packages use "ink" itself as a dependency in package.json, and that means ink-spinner (e.g.) will try to use the officially-released rendering logic as opposed to whatever modified version your solution provides. (I ran into this problem when I released Add overflow prop to Box component #393 as a fork.) Solutions that require forking any/all component packages are not acceptable, and my use case doesn't afford leveraging systems like yarn's resolutions config. (I intend to use this work in a dependent application released to NPM and installable with npm install, so yarn-specific niceties are sadly just not going to cut it.)

  • A4. Solution includes automated tests and follows consistent style and quality alongside the rest of Ink's codebase.

This is about the extent of what I can think of as necessary, but of course I understand that there may be ambiguity or technical limitations when it comes to the details. Please don't hesitate to communicate any comments, questions, or concerns.

Thank you to everyone who has thus far applied to work on this bounty! I will continue reviewing each of your applications and will respond to each individually in the next 48 hours. ❤️

@gitcoinbot
Copy link

@developerfred Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • reminder (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@developerfred
Copy link

Hi @gitcoinbot my code will be here https://replit.com/@codingsh/poc-ink-box-upgrade

developerfred added a commit to developerfred/ink-box-upgrade that referenced this issue Apr 21, 2021
@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


Work for 1.0 ETH (2311.76 USD @ $2425.56/ETH) has been submitted by:


@developerfred
Copy link

@gnidan there is a way for me to get the height of the elements with this hook, I will write more test and improve the poc.

@gnidan
Copy link
Author

gnidan commented Apr 22, 2021

Awesome @developerfred, glad to hear about progress!

In the meantime, I grabbed the relevant stuff from my use case and made a repo for it at gnidan/ink-scroller. There's something strange going on with left/right scrolling, but I'm less focused on that, since I just haven't looked into it.

Point of that repo: I want to be able to stop scrolling at the end of the text :)

@dustinlacewell
Copy link

dustinlacewell commented Jul 8, 2021

This is the current behavior I'm seeing

2021-07-08-22-50-43

@navxio
Copy link

navxio commented Jan 12, 2022

@developerfred any luck? Can I offer (free) help?

@developerfred
Copy link

@navxio sure!

@vadimdemedes
Copy link
Owner

I think it would be interesting to have that as a separate npm package, but I'm not sure I'd want to have that in Ink itself. If you have a complicated CLI that requires scrolling areas, you might be better of with ncurses-based alternative like blessed.

@vadimdemedes vadimdemedes changed the title Update Box to support overflow and scrolling Support Box overflow and scrolling Jan 31, 2022
@zach-is-my-name
Copy link

zach-is-my-name commented Feb 12, 2022

I think it would be interesting to have that as a separate npm package, but I'm not sure I'd want to have that in Ink itself. If you have a complicated CLI that requires scrolling areas, you might be better of with ncurses-based alternative like blessed.

The issue with blessed and similar variants is we don't get the declarative dom-like environment that we get here... as it stands and (please someone correct me if I'm wrong), Ink is the only project that makes building a CLI or TUI more or less like building for the web...

And I believe that's hugely important because we've come to realize the terminal offers one of the most productive, distraction free environments in existence... a turn-key solution to scrolling is absolutely fundamental in my opinion. Thanks for listening

@gnidan
Copy link
Author

gnidan commented Feb 12, 2022

Glad to see some activity on this issue. I'll review the acceptance criteria and state of current efforts. I still want this, and I'm still willing to spend the listed bounty!

Thanks for your insight @vadimdemedes and @zach-is-my-name 🙏

@gnidan
Copy link
Author

gnidan commented Feb 13, 2022

I think it would be interesting to have that as a separate npm package, but I'm not sure I'd want to have that in Ink itself.

@vadimdemedes I remember from my exploration into this issue that we can't get a separate NPM package to work without a change to Ink's rendering methods... since Ink is responsible for doing all the layout math, it needs to keep track of the "full size" dimensions as well as the computed dimensions on-screen (what I call "natural size" vs. "fixed size" above), but the problem is that there's no way (right now) to create a react hook to report the natural size dimensions. It looks like this information gets lost somewhere deep inside the renderer (or worse, inside the Yoga/flexbox computations).

This stuff is all very confusing to me, however, since Yoga doesn't (didn't?) have good API documentation. Maybe I'm wrong.

Hi @gitcoinbot my code will be here https://replit.com/@codingsh/poc-ink-box-upgrade

@developerfred is this still current? It seems rather barebones, and it doesn't currently even run at all (looks like you have a syntax error).


Anyway, I notice there is some movement on #412 and also I see #472 is new since I've spent effort investigating the state of this issue. @vadimdemedes could you please comment on what a path forward might look like? I'm fine if it is actually possible to achieve this "natural size" calculation in a separate NPM package, but I just don't see how that's possible.

If further coordination across existing PRs is necessary, I could potentially double the bounty and split it up across contributors/maintainers, since I really want this behavior, but mind you I'm only working with my personal funds here. But if this is a multi-person effort, I do want to recognize that and reward everyone's efforts. This behavior is the only thing stopping me from using Ink for a number of projects of varying sizes, and I can't move forward on those projects in a way that satisfies me (because, well, who wants to use blessed when ink exists ;)

@zach-is-my-name
Copy link

#521 what say you @gnidan?

@gnidan
Copy link
Author

gnidan commented Jun 14, 2022

#521 what say you @gnidan?

Love it. Thank you @zach-is-my-name. I'll watch that issue to see how this develops.

@vadimdemedes
Copy link
Owner

Now that Box supports overflow="hidden", it's possible to create a scrollable box without modifying Ink internals.

Here's an example ScrollArea component I created for scrolling vertically positioned items:

const reducer = (state, action) => {
	switch (action.type) {
		case 'SET_INNER_HEIGHT':
			return {
				...state,
				innerHeight: action.innerHeight
			};

		case 'SCROLL_DOWN':
			return {
				...state,
				scrollTop: Math.min(
					state.innerHeight - state.height,
					state.scrollTop + 1
				)
			};

		case 'SCROLL_UP':
			return {
				...state,
				scrollTop: Math.max(0, state.scrollTop - 1)
			};

		default:
			return state;
	}
};

function ScrollArea({height, children}) {
	const [state, dispatch] = React.useReducer(reducer, {
		height,
		scrollTop: 0
	});

	const innerRef = React.useRef();

	React.useEffect(() => {
		const dimensions = measureElement(innerRef.current);

		dispatch({
			type: 'SET_INNER_HEIGHT',
			innerHeight: dimensions.height
		});
	}, []);

	useInput((_input, key) => {
		if (key.downArrow) {
			dispatch({
				type: 'SCROLL_DOWN'
			});
		}

		if (key.upArrow) {
			dispatch({
				type: 'SCROLL_UP'
			});
		}
	});

	return (
		<Box height={height} flexDirection="column" overflow="hidden">
			<Box
				ref={innerRef}
				flexShrink={0}
				flexDirection="column"
				marginTop={-state.scrollTop}
			>
				{children}
			</Box>
		</Box>
	);
}

Usage:

<Box flexDirection="column" paddingBottom={1}>
	<ScrollArea height={6}>
		{Array.from({length: 20})
			.fill(true)
			.map((_, index) => (
				<Box key={index} flexShrink={0} borderStyle="single">
					<Text>Item #{index + 1}</Text>
				</Box>
			))}
	</ScrollArea>
</Box>
CleanShot.2023-04-24.at.11.54.09.mp4

I think a similar component can be made to support horizontally scrollable items as well.

The problem I didn't account here is focus management. ScrollArea listens to arrow up/down keys via useInput, so if we were to also render a SelectInput at the same time, which also listens to the same keys, both of these components would react to them. ScrollArea would scroll and SelectInput would select a different item.

We could make ScrollArea a focusable component by using useFocus, so it'd only scroll the items if ScrollArea is focused.

However, this wouldn't work properly if we rendered a ScrollArea with TextInput, which is also a focusable component, but it doesn't listen to arrow up or arrow down keys. So if TextInput was placed inside a ScrollArea and would be focused, pressing arrow up/down keys wouldn't scroll the ScrollArea.

One of the options would be to explicitly document that ScrollArea is supposed to scroll non-focusable elements. Another option would be to have an independent focus management for ScrollArea components, separate from useFocus and useFocusManager that's used by user-land components.

Thinking out loud. Let me know your thoughts on this.

gnidan added a commit to gnidan/ink-scroller that referenced this issue Apr 27, 2023
@gnidan
Copy link
Author

gnidan commented Apr 27, 2023

This is great! Thanks for getting back to this @vadimdemedes. I just adapted your example for my old https://github.com/gnidan/ink-scroller repo and I am able to get up/down scrolling to work perfectly, with limits for when the end of content gets reached. (The Math.max() business seems a bit prone to off-by-one errors, but I think that's just the way I have it set up. Not a real issue, I don't think.)

I'm not super concerned with the focus-based scrolling, since I think there's a relatively straightforward solution to check for own focus, plus pass some handleChildFocus function as a prop to the children (or similar). It's a bit hacky, but anything more robust would probably require Ink to do more heavy-lifting, and that seems excessive for now. Maybe in the future you'll want to address this limitation? This limitation is fine for me right now, at least.

One question, since my example ink-scroller repo uses random paragraphs of lorem-ipsum text. When I scroll left/right, Ink re-renders the paragraphs every time I scroll (i.e., it computes the word wrapping every scroll action, which makes individual lines re-wrap differently).

See GIF of what I mean

scroller

It's possible that I just have a bug, since it's been awhile since I've looked at my example code. But in case you might know... should I just plan to add line-breaks manually and not rely on Ink's wrapping logic?

@gnidan
Copy link
Author

gnidan commented Apr 27, 2023

Regarding the bounty portion of this issue, I consider the matter resolved. I can use the regular "ink" NPM package to get <Box /> with both overflow support and support for bounded scrolling, which is exactly what I have needed!

With pleasure, I'd like to remit the 1 ETH funds associated with this bounty.

@developerfred: Although your solution did not meet acceptance criteria, I'm sending you a 0.1 ETH tip for your attempt when I first opened this. You're already in the Gitcoin system, so I was able to use the Gitcoin webapp UI to issue this payout... unfortunately I didn't check the gas price, so that transaction may get dropped. Don't worry; I will monitor this and make sure you get paid. Edit: I sent you the 0.1 ETH here. Thank you!

@tin-t: I'd like to offer you a 0.2 ETH tip as gratitude for your work on the initial implementation of <Box overflow /> support in #393.

@vadimdemedes: Between #502 and #432 (comment), your work clearly meets the acceptance criteria, and thus you deserve the 1.0 ETH! I would like to send this to you.

But, for both @tin-t and @vadimdemedes... I don't know how to issue you these payments.

  • It might be easiest if you make https://bounties.gitcoin.co/ accounts and sign up to work on this bounty (https://bounties.gitcoin.co/issue/25559). It looks like it's pretty easy for me to send arbitrary amounts to anyone who is in their system and associated with this bounty (edit: this last part doesn't matter... I can send a tip to any Gitcoin account, it seems).
  • Alternatively, just send me your Ethereum address (you can paste here in a comment, send it to me in a Twitter DM, or we can find another means of communicating.) I can just remit the funds directly and figure out Gitcoin's commission separately.

I understand that the two of you might not be prepared to receive ETH. I apologize if this is the case, but unfortunately (for budgeting and tax reasons), I can't issue the payout in a different currency other than ETH. I am happy to walk you through the process of making an Ethereum account and helping you find a reputable fiat off-ramp that you can use if you want to convert the funds into your local currencies. Please let me know.

For housekeeping purposes, I'll plan to leave both this GH issue and the associated bounty open until it's fully paid. I worry that something on the Gitcoin side will break otherwise...

Anyway, thank you everyone who contributed to this issue, and enormous thanks to you @vadimdemedes for all your work on Ink, really :). I'm looking forward to picking up where I left off when I first got blocked by this overflow/scrolling thing.

Cheers all!

Edit: @vadimdemedes it looks like you might already have a Gitcoin account? But no Ethereum address associated, so I'll still wait to hear from you.

@vadimdemedes
Copy link
Owner

@gnidan That's very generous of you, thank you! If you could send it either to 0xa1b1bbB8070Df2450810b8eB2425D543cfCeF79b or 0x93Bda139023d582C19D70F55561f232D3CA6a54c, that'd be awesome 💛

One question, since my example ink-scroller repo uses random paragraphs of lorem-ipsum text. When I scroll left/right, Ink re-renders the paragraphs every time I scroll (i.e., it computes the word wrapping every scroll action, which makes individual lines re-wrap differently).

I think it can be solved by setting a fixed width to the Box that wraps the children (not the root Box).

@gnidan
Copy link
Author

gnidan commented Apr 28, 2023

@gnidan That's very generous of you, thank you! If you could send it either to 0xa1b1bbB8070Df2450810b8eB2425D543cfCeF79b or 0x93Bda139023d582C19D70F55561f232D3CA6a54c, that'd be awesome 💛

Sent. I hope it makes a difference!

I think it can be solved by setting a fixed width to the Box that wraps the children (not the root Box).

This works, thank you!

For housekeeping purposes, I'll plan to leave both this GH issue and the associated bounty open until it's fully paid.

I closed the bounty on the Gitcoin side, and I'm going to close this issue now.

(@tin-t it looks like you have not been active on Github in awhile, but if you see these notifications, please find a way to reach out to me so I can make good on my stated offer above! Hopefully I will see any notifications here, but if not, my username is @gnidan on most social media platforms)

@gnidan gnidan closed this as completed Apr 28, 2023
@vadimdemedes
Copy link
Owner

Thanks @gnidan, every dollar makes the Ukrainian victory closer 💛

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants