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

Default margins #152

Closed
duncanbeevers opened this issue Mar 12, 2019 · 11 comments
Closed

Default margins #152

duncanbeevers opened this issue Mar 12, 2019 · 11 comments
Labels

Comments

@duncanbeevers
Copy link

Margins seem to be behaving inconsistently.

Let me know if I'm misunderstanding something here, but it seems like the behavior of these two examples should be equivalent.

Example 1 – Correct margins

function App() {
  const [isToggled, setToggle] = useState(true);

  useEffect(() => {
    const interval = setInterval(() => {
      setToggle(!isToggled);
    }, 1000);

    return () => {
      clearInterval(interval);
    };
  }, [setToggle, isToggled]);

  if (isToggled) {
    return <Box margin={0}>ON</Box>;
  } else {
    return <Box margin={1}>OFF</Box>;
  }
}

2019-03-11 at 8 08 PM

Example 2 – Incorrect margins

  if (isToggled) {
    return <Box>ON</Box>;
  } else {
    return <Box margin={1}>OFF</Box>;
  }

2019-03-11 at 8 06 PM

Is this expected behavior? Am I holding it wrong?

@vadimdemedes
Copy link
Owner

Yep, output in your demos is correct. Keep in mind that Ink operates in the terminal, where there are no pixels. Vertical and horizontal margins will never look the same, because Ink uses a space for horizontal margins and an empty line for vertical margins.

@vadimdemedes
Copy link
Owner

You can reproduce the same output via console.log('\n ON').

@sindresorhus
Copy link
Collaborator

sindresorhus commented Mar 13, 2019

@vadimdemedes We should at least document that. I think horizontal margin needs to be 3 times vertical margin to look even (at least that's what I'm using in boxen).

@vadimdemedes
Copy link
Owner

We should at least document that

Agree.

I think horizontal margin needs to be 3 times vertical margin to look even (at least that's what I'm using in boxen).

Hm, how would you have 1 space margin in that case? E.g.

<Box>
	<Box marginRight={1}>X</Box>
	Y
</Box>

=> X Y

@sindresorhus
Copy link
Collaborator

Hm, how would you have 1 space margin in that case? E.g.

You wouldn't. One vertical space is the same as three horizontal spaces.

@vadimdemedes
Copy link
Owner

But if there was no ability to create 1 or 2 space horizontal margins, you wouldn't be able to create a layout like https://twitter.com/vadimdemedes/status/1103345757434736641 without using {' '}, which is kind of the whole point of having margins in Ink.

@sindresorhus
Copy link
Collaborator

sindresorhus commented Mar 13, 2019

I'm not saying that Ink should handle this (It shouldn't). Just sharing my experience from creating boxen.

@duncanbeevers
Copy link
Author

My concern here isn't about the specific size. Rather, when an explicit 0 margin is specified output is rendered flush with the terminal edge. When no margin is specified, the output is first rendered flush with the terminal edge, then when bumped away from the edge with 1 margin, and then remains fixed at the 1 margin position when rendering switches back to the unspecified margin mode.

@vadimdemedes
Copy link
Owner

Oooh I didn't get that. Will look into it, @duncanbeevers!

@vadimdemedes
Copy link
Owner

This seems to be the same issue as #228, where Ink didn't unset props properly. Will be fixed in the next release.

@vadimdemedes
Copy link
Owner

Fixed in 2.5.0.

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

No branches or pull requests

3 participants