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

Linkstate refactor #609

Merged
merged 7 commits into from
Sep 8, 2023
Merged

Linkstate refactor #609

merged 7 commits into from
Sep 8, 2023

Conversation

jvorob
Copy link
Contributor

@jvorob jvorob commented Aug 4, 2023

Big refactor to LinkingState

  • pulls out descriptive/user-facing info into LinkingState

Adds Linkinfo to SelectBy/"Selector For" UI in right panel

Bugfix: Disables linking from Attachment columns

Bugfix/Behavior change: changed linking with empty RefLists to better match behavior of refs.

  • (for context: Linking by a blank Ref filters to show records with a blank value for that Ref)
  • Previously this didn't work with RefLists. Linking from a blank refList would show no records (except in some cases involving summary tables)
  • Fixed this so that linking by a blank val consistently means "show all records where the corresponding col is blank"

Tweaks to buildViewSectionDom, widgetTitle DOM
previously the title dom element (testid('viewsection-title')) was flex-grow 1, so all the blank space in the widget-title-row was part of the title element (but only the actual title was clickable). This swaps it so that the title doesn't grow, and instead the space is taken up by a small div which stretches to fill available space. Makes it easier to add other dom around the title

@jvorob jvorob added the preview Launch preview deployment of this PR label Aug 4, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2023

Deployed as https://grist-linkstate-refactor.fly.dev (until 2023-09-03T18:28:46.734Z)

@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2023

Deployed as https://grist-linkstate-refactor.fly.dev (until 2023-09-04T01:35:08.733Z)

@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2023

Deployed as https://grist-linkstate-refactor.fly.dev (until 2023-09-04T01:38:08.071Z)

@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2023

Deployed as https://grist-linkstate-refactor.fly.dev (until 2023-09-04T03:13:29.821Z)

@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2023

Deployed as https://grist-linkstate-refactor.fly.dev (until 2023-09-04T03:32:32.394Z)

@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2023

Deployed as https://grist-linkstate-refactor.fly.dev (until 2023-09-04T04:58:54.984Z)

@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2023

Deployed as https://grist-linkstate-refactor.fly.dev (until 2023-09-04T05:21:14.672Z)

@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2023

Deployed as https://grist-linkstate-refactor.fly.dev (until 2023-09-04T07:17:55.969Z)

@github-actions
Copy link
Contributor

github-actions bot commented Aug 7, 2023

Deployed as https://grist-linkstate-refactor.fly.dev (until 2023-09-06T15:49:39.412Z)

@github-actions
Copy link
Contributor

github-actions bot commented Aug 7, 2023

Deployed as https://grist-linkstate-refactor.fly.dev (until 2023-09-06T16:11:33.612Z)

@github-actions
Copy link
Contributor

github-actions bot commented Aug 7, 2023

Deployed as https://grist-linkstate-refactor.fly.dev (until 2023-09-06T16:32:16.696Z)

@jvorob jvorob marked this pull request as ready for review August 7, 2023 16:51
@jvorob jvorob force-pushed the linkstate-refactor branch 2 times, most recently from 78c01cc to b78339f Compare August 8, 2023 16:10
@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 2023

Deployed as https://grist-linkstate-refactor.fly.dev (until 2023-09-07T16:08:42.671Z)

@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 2023

Deployed as https://grist-linkstate-refactor.fly.dev (until 2023-09-07T16:14:25.270Z)

@paulfitz paulfitz requested a review from berhalak August 9, 2023 18:31
app/client/ui/RightPanel.ts Outdated Show resolved Hide resolved
app/client/ui/RightPanel.ts Outdated Show resolved Hide resolved
app/client/components/LinkingState.ts Outdated Show resolved Hide resolved
app/client/components/LinkingState.ts Outdated Show resolved Hide resolved
app/client/components/LinkingState.ts Outdated Show resolved Hide resolved
app/client/ui/RightPanel.ts Outdated Show resolved Hide resolved
app/client/ui/RightPanel.ts Outdated Show resolved Hide resolved
app/client/ui/WidgetTitle.ts Outdated Show resolved Hide resolved
app/client/ui/selectBy.ts Outdated Show resolved Hide resolved
app/client/ui/selectBy.ts Outdated Show resolved Hide resolved
@berhalak
Copy link
Contributor

Can you add tests for the bugs you have fixed?

@github-actions
Copy link
Contributor

Deployed as https://grist-linkstate-refactor.fly.dev (until 2023-09-20T23:02:11.574Z)

@github-actions
Copy link
Contributor

Deployed as https://grist-linkstate-refactor.fly.dev (until 2023-09-21T07:11:59.098Z)

@github-actions
Copy link
Contributor

Deployed as https://grist-linkstate-refactor.fly.dev (until 2023-09-21T07:22:29.692Z)

Big refactor to LinkingState
    pulls out descriptive/user-facing info into LinkingState

Adds Linkinfo to SelectBy/"Selector For" UI in right panel

Bugfix: Disables linking from Attachment columns

Bugfix/Behavior change: changed linking with empty RefLists to better
match behavior of refs.

    (for context: Linking by a blank Ref filters to show records with a
blank value for that Ref)
    Previously this didn't work with RefLists. Linking from a blank
refList would show no records (except in some cases involving summary
tables)
    Fixed this so that linking by a blank val consistently means "show
all records where the corresponding col is blank"

Tweaks to buildViewSectionDom, widgetTitle DOM
previously the title dom element (testid('viewsection-title')) was
flex-grow 1, so all the blank space in the widget-title-row was part of
the title element (but only the actual title was clickable). This swaps
it so that the title doesn't grow, and instead the space is taken up by
a small div which stretches to fill available space. Makes it easier to
add other dom around the title
Fixed bug when lfilters=EmptyFilterState
Fixed computed leak on linkTypeDescription
Fixed "srcSection disposed" bug with linkTypeDescription
Fixed disposables in LinkingState: _update
Fixed imports (sorted, no relative paths)
Moved advancedLinkInfo isCollapsed into sessionObs
Indentation
Pulled out custom links into own if{} case
Removed spurious `owner` param in buildLinkInfo and co.
Css cleanup
Undid viewsection title flex fix
Changed all == to ===
Many comments, better explains linking logic
@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2023

Deployed as https://grist-linkstate-refactor.fly.dev (until 2023-10-05T23:25:11.607Z)

Previously, I only updated the _filterState computed once,
but the intent was for the Computed of merged filters should update
_filterState whenever it changed.

This also led to a bug where summary-linking wouldn't work (only
appeared to show up after rebasing onto latest main branch)
@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2023

Deployed as https://grist-linkstate-refactor.fly.dev (until 2023-10-05T23:39:57.219Z)

Copy link
Contributor

@berhalak berhalak left a comment

Choose a reason for hiding this comment

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

Thanks.
Some comments and linting suggestions only.

app/client/components/LinkingState.ts Show resolved Hide resolved
app/client/components/LinkingState.ts Outdated Show resolved Hide resolved
app/client/components/LinkingState.ts Outdated Show resolved Hide resolved
app/client/components/LinkingState.ts Outdated Show resolved Hide resolved
app/client/components/LinkingState.ts Outdated Show resolved Hide resolved
app/client/components/LinkingState.ts Show resolved Hide resolved
app/client/components/LinkingState.ts Show resolved Hide resolved
app/client/components/LinkingState.ts Outdated Show resolved Hide resolved
app/client/components/LinkingState.ts Outdated Show resolved Hide resolved
app/client/components/LinkingState.ts Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2023

Deployed as https://grist-linkstate-refactor.fly.dev (until 2023-10-08T17:57:57.384Z)

@jvorob jvorob merged commit f8c1bd6 into main Sep 8, 2023
13 checks passed
@jvorob jvorob deleted the linkstate-refactor branch September 11, 2023 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Launch preview deployment of this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants