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

Allow table to be horizontally scrollable with full-width columns #83

Closed
candrewlee14 opened this issue Jun 2, 2022 · 29 comments · Fixed by #87
Closed

Allow table to be horizontally scrollable with full-width columns #83

candrewlee14 opened this issue Jun 2, 2022 · 29 comments · Fixed by #87
Assignees
Labels
enhancement New feature or request

Comments

@candrewlee14
Copy link

There should be a way to disallow columns being truncated like this, and instead allow the table to stretch horizontally offscreen, which would then be able to be scrolled left and right with the arrow keys. There's an example output below for a table with many columns where truncation makes the table pretty much useless.

171625774-93a43670-106f-468d-8d04-deaa46be7ca6

@Evertras
Copy link
Owner

Evertras commented Jun 4, 2022

Yep, that's a useless table!

I was starting to type up that the easiest solution would be to render the table in a horizontally scrollable view component, but I'm looking at the footer and realizing that won't work as the footer should remain static even as the rest of the table scrolls.

This is a reasonable feature request, let me think about how this might work.

@Evertras Evertras added the enhancement New feature or request label Jun 4, 2022
@candrewlee14 candrewlee14 changed the title Allow table to be horizontally scrollable with full-wdith columns Allow table to be horizontally scrollable with full-width columns Jun 4, 2022
@candrewlee14
Copy link
Author

candrewlee14 commented Jun 4, 2022

I've actually implemented a pretty simple solution for it here: https://github.com/influxdata/influx-cli/pull/393/files#diff-69a5ed4eca0dfe3e0e3d0a4f8a6552f9eafe6fd6fddcac1c1199176fab421578
Left and right arrow keys scroll between the visible columns.
I didn't implement everything ideally, but it gets the job done for my use-case.
Hopefully that helps with a little inspiration, and maybe you can lower some of that code into the library.

@Evertras
Copy link
Owner

Evertras commented Jun 4, 2022

Thanks for the link! I've actually already started working on this with a similar idea of column offsets, but with a bit more visual notice that scrolling is occurring. I'm going to use shift+left and shift+right for scrolling as left/right are the default keys for paging, but that can be set through the keymap.

@Evertras Evertras self-assigned this Jun 4, 2022
@Evertras
Copy link
Owner

Evertras commented Jun 4, 2022

I've finished the first pass of horizontal scrolling, but I want to add column freezing as well - as in your example where you always have the index column. This issue should remain open until that's done at least.

@Evertras
Copy link
Owner

Evertras commented Jun 4, 2022

I've added horizontal freezing, so that you can freeze the first N columns, which should cover your use case if I understood your linked code above. Let me know if this is reasonable or if there's other things that need to be added for this to be useful! This has been released as v0.13.0.

It looks like Github automatically closed this issue due to one of the links I made, but I'd rather you close this yourself if you confirm that the new version solves the issue. If it doesn't, please let me know what else you think may need to be done.

@Evertras Evertras reopened this Jun 4, 2022
@candrewlee14
Copy link
Author

candrewlee14 commented Jun 4, 2022

Awesome work! I do have a couple thoughts:

  • For the static footer, I think its width should be the full width of the rows. If I scroll to the right, the static footer still stays full width of the screen which makes the table look a little strange. Is there an easy way to do keep the row and footer size in sync?

Screen Shot 2022-06-04 at 10 01 20 AM

  • Is there a method to get the width of the table?

@candrewlee14
Copy link
Author

I also think this behavior is a little strange. I like the small caret column on the left, but when it's on the right side, it is much bigger and visually seems off.
Screen Shot 2022-06-04 at 10 25 46 AM
Screen Shot 2022-06-04 at 10 22 49 AM

@Evertras
Copy link
Owner

Evertras commented Jun 4, 2022

The footer thing is definitely a bug, I was focusing on all the cases where the table was larger, and not smaller... this should be fixed in #90 , will release after addressing the other issue that you posted as I was typing this as well.

For getting the total width, there are some internal calculations that are done to try and track the width for caching purposes, but it's not terribly nice for exposing to the outside world. What are you trying to do? A hacky way would be to render the table and run lipgloss.Width() on it to get the actual rendered width, if that would be useful.

@Evertras
Copy link
Owner

Evertras commented Jun 4, 2022

For the right side carat, an easy fix is to force the alignment to be to the right. A trickier question is whether or not the column should always just be one single character wide, because this may have unexpected impacts on the footer width. Open to discussion on this one.

@candrewlee14
Copy link
Author

candrewlee14 commented Jun 4, 2022

I think in the name of visual and scrolling consistency, it makes sense for the caret to be one character on either side.
It'd be nice that if you continue to scroll right, the caret column won't keep changing sizes, which is distracting from the actual data we want to show. But I do see what you're saying about the footer width. Like if the footer suddenly wraps, it would make a new line and the height wouldn't be valid anymore.

@Evertras
Copy link
Owner

Evertras commented Jun 4, 2022

I changed my mind on releasing and released v0.13.1 with the footer fix as I need to crash to sleep for the night, and want to play with the carat width a bit more tomorrow. Let me know if the footer works better for you with that.

I'll give this some thought of how to make the single character work while also preserving a potentially long footer as in your screenshot.

@candrewlee14
Copy link
Author

Also, another footer width edge case to handle:
When you scroll right and static footer wraps to create a new line, it means that the line count to clear changes from what the original calculation was. This makes it leaves residual top borders that stack every time a resize or scroll happens.

Screen Shot 2022-06-04 at 10 40 06 AM

@Evertras
Copy link
Owner

Evertras commented Jun 5, 2022

I think I want to optimize for the table width being constant. This would add blank space at the end of the table if there are still > carets to display, and we could right-align the carets so that they don't jump around. This would be similar to how it is now, just right-aligned. The upside is the footer is safe. The downside is the extra padding might look a bit odd, as you pointed out above. Do you think this is a reasonable tradeoff? I'm open to ideas/discussion here for sure.

In the meantime I'm going to go right-align the carets anyway, as I don't think there's a world where weirdly centered carets are a thing anyone wants. We can adjust later, maybe with some configuration options. I'm also going to adjust the scrolling so that it only scrolls until there's nothing left to the right, and stops. This won't fix all footer resizing issues but will at least give some help there.

@Evertras
Copy link
Owner

Evertras commented Jun 5, 2022

I've implemented the two things as discussed above and released as v0.13.2. Give it a try and let me know if it helps, still open to discussing longer term formatting issues regardless.

@candrewlee14
Copy link
Author

candrewlee14 commented Jun 5, 2022

I think that's a good call to optimize for consistency...Although I've tried out this new version, and my keybindings for left and right aren't doing anything anymore. Not sure if I'm missing something, but I now can't scroll either direction even when the caret columns are showing.

@Evertras
Copy link
Owner

Evertras commented Jun 6, 2022

Were you using shift? The default keybind is shift plus arrows, but you can override that by changing the keymap.

@candrewlee14
Copy link
Author

Shift arrows did not work, and my overrides for left and right didn't work after that either.

@Evertras
Copy link
Owner

Evertras commented Jun 6, 2022

Is the table set to be focused? As in myTable.Focused(true) is used when created?

@candrewlee14
Copy link
Author

Yeah, so I was able to page up and down, but not scroll left and right.

@Evertras
Copy link
Owner

Evertras commented Jun 6, 2022

That’s a bit terrifying. Let me double check some things when I get some time. Are you able to run the scrolling example in this repo?

@Evertras
Copy link
Owner

Evertras commented Jun 6, 2022

Another question that came to mind, what version of Bubble Tea are you using? I did have to upgrade the Bubble Tea dependency in order to properly use shift+left/right, I'm wondering if there's a conflict there.

@Evertras
Copy link
Owner

Evertras commented Jun 6, 2022

Additionally, have you tried remapping the keys to something else to see if that might work?

@candrewlee14
Copy link
Author

candrewlee14 commented Jun 6, 2022

Ok, I've figured out that remapping is causing problems but I can get the scrolling working back with version 0.13.1. If I have no WithKeyMap, the shift+left/right commands work great. But if I have WithKeyMap and pass in the default keybindings, or I try to override the ScrollLeft/Right keybindings, then I can't scroll left & right.

Then when I upgrade to 0.13.2, the same code doesn't work anymore.
I think it's got something to do with the recalculateLastHorizontalColumn function, because the scrollRight function doesn't make it through the if statement.

@Evertras
Copy link
Owner

Evertras commented Jun 7, 2022

Oof. I had trouble making that particular function work smoothly, and I thought I had fixed the edge cases but it's likely you're running into that. Could you give me the dimensions of the table you're using so I can test locally? As in, column widths and which are frozen?

@Evertras
Copy link
Owner

Evertras commented Jun 7, 2022

I've tested a bit with the keybindings and they seem to work all right, but I may have a clue as to what's happening with your situation. It's important to note that rebinding them to left or right won't actually work if you only replace the scrolling keybinds... you will also need to replace the PageUp/PageDown keybinds. Otherwise the left and right catch first on the page up/down and they're never tested for scrolling, since a match is already found.

I've passed in the default keybindings with WithKeyMap in the scrolling example successfully and changed the keybinding successfully as well. If there's something breaking with the default keymap being applied then I may need a code sample of non-working code to get any further with that.

@Evertras
Copy link
Owner

Evertras commented Jun 11, 2022

I've fixed the keymap behavior to properly run all actions instead of short-circuiting, so your left/right rebinds should now work and are verified as unit tests with this change. This is released in v0.13.6

@Evertras
Copy link
Owner

I finally got a good excuse to try out fuzzing, and it helped me find the edge cases you were likely running into. I was doing an incorrect calculation regarding the frozen first column, which has been removed and now the fuzzer seems quite happy with all kinds of permutations. Try v0.13.7 to see if that works better for you regarding scrolling all the way to the right again.

@candrewlee14
Copy link
Author

Glad to hear! I'll try it out a little later today and let you know.

@candrewlee14
Copy link
Author

candrewlee14 commented Jun 12, 2022

Alright, I've come back and found that it works! Thanks for your diligence on this.

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

Successfully merging a pull request may close this issue.

2 participants