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 pinning to table #3383

Merged
merged 14 commits into from
Nov 9, 2023
Merged

Add pinning to table #3383

merged 14 commits into from
Nov 9, 2023

Conversation

djbarnwal
Copy link
Member

@djbarnwal djbarnwal commented Nov 3, 2023

Add pinning to dimension table as per https://www.notion.so/rilldata/Time-Dimension-Detail-Design-Mocks-and-Specification-2dd22773d6e8479ca8d297f95877c669

Couple of things to note -

  1. Our custom tooltips can't be used as they are written in svelte. The table works directly with HTML strings. Not sure what our strategy should be here. Tried adding tooltips using CSS but they were buggy and their positioning doesn't work well as specified in mock. (A tooltip on the right side should be possible though using just CSS)
  2. The totals row is not fixed at the moment. Making it fixed would require a big refactor across all the TDD components (because of the way regular-table works around fixed data). I would propose moving the pin icon on the top beside the dimension label instead of it currently being beside the "Total" label.

@djbarnwal djbarnwal marked this pull request as ready for review November 7, 2023 14:09
@djbarnwal djbarnwal requested a review from ericpgreen2 November 7, 2023 14:09
@djbarnwal djbarnwal self-assigned this Nov 7, 2023
@ericpgreen2
Copy link
Contributor

100% agree on your proposal to move the pin icon one row up into the header row.

@ericpgreen2
Copy link
Contributor

I'm not sure if it's related to this PR, but one UX problem I'm seeing for the first time is that when I move my browser focus to a different tab, then return focus to the Rill (Developer) tab where I have the TDD view open, I hit a loading state where both the chart and the time series table have spinners.

I'd expect any data refetch to happen in the background, rather than tear down the current data.

Copy link
Contributor

@ericpgreen2 ericpgreen2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving from a code standpoint 👍

@djbarnwal
Copy link
Member Author

I'm not sure if it's related to this PR, but one UX problem I'm seeing for the first time is that when I move my browser focus to a different tab, then return focus to the Rill (Developer) tab where I have the TDD view open, I hit a loading state where both the chart and the time series table have spinners.

I'd expect any data refetch to happen in the background, rather than tear down the current data.

I think that's a issue for the whole dashboard. This is also present in the overview screen. This is because of how reconcile works when a user moves back to an inactive tab. For sure we can improve the experience by having better loaders and blank placeholders

@djbarnwal djbarnwal merged commit 121c8d5 into main Nov 9, 2023
3 checks passed
@djbarnwal djbarnwal deleted the tdd/pin branch November 9, 2023 15:19
@bcolloran
Copy link
Contributor

hey @ericpgreen2 and @djbarnwal, I know this is merged, but I want to flag an issue for you both in the future.

@djbarnwal, I have seen several places in the app where you use -1 as a sentinel value for invalid data. Sentinel values are not a best practice in languages where nullable values are available, because they obscure the semantics of the value for other users of the code (especially when undocumented, as is the case for pinIndex and the other places where this pattern is used). The only way that another member of the team will become aware that this invalid value may show up is if they happen to notice the "-1"s floating around, which may not be present in all the cases where this value is referenced.

The preferred approach would be to use the type pinIndex: number | null or pinIndex: number | undefined rather than the current pinIndex: number. This makes it clearer to other teammates (and your future self) that not everything that shows up in a pinIndex value can necessarily be used as a valid index.

(also fyi @AdityaHegde in case you see this pattern in a review)

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