search

vadimdemedes / ink

🌈 React for interactive command-line apps
https://term.ink
MIT License
27.13k stars 609 forks source link

Support `Box` overflow and scrolling #432

Closed gnidan closed 1 year ago

gnidan commented 3 years ago

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:

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.

https://github.com/vadimdemedes/goodness-squad/issues/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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Here's a solution based on #393 : https://gist.github.com/chrisirhc/bf3f973e5d4db2cbc96d95d1f023f0ad

gnidan commented 3 years ago

@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 commented 3 years ago

Acceptance criteria

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 commented 3 years ago

@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!

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

developerfred commented 3 years ago

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

gitcoinbot commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

This is the current behavior I'm seeing

2021-07-08-22-50-43

navxio commented 2 years ago

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

developerfred commented 2 years ago

@navxio sure!

vadimdemedes commented 2 years ago

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.

zach-is-my-name commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

521 what say you @gnidan?

gnidan commented 2 years ago

521 what say you @gnidan?

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

vadimdemedes commented 1 year ago

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>

https://user-images.githubusercontent.com/697676/233948081-0d1823a4-9af6-4785-a38d-fe7b4c4b3721.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 commented 1 year ago

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](https://user-images.githubusercontent.com/151065/234746194-02568431-dc77-4ddc-9620-9503ca0cddc0.gif)

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 commented 1 year ago

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 https://github.com/vadimdemedes/ink/issues/432#issuecomment-1519671092, 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.

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 commented 1 year ago

@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 commented 1 year ago

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

vadimdemedes commented 1 year ago

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