Skip to content

feat: De-globalize JSAPI in IrisGrid package#1262

Merged
vbabich merged 16 commits intodeephaven:mainfrom
vbabich:jsapi-deglobalize-irisgrid
May 8, 2023
Merged

feat: De-globalize JSAPI in IrisGrid package#1262
vbabich merged 16 commits intodeephaven:mainfrom
vbabich:jsapi-deglobalize-irisgrid

Conversation

@vbabich
Copy link
Copy Markdown
Collaborator

@vbabich vbabich commented May 4, 2023

  • De-globalize JSAPI in the IrisGrid package.
  • De-globalize JSAPI in DateUtils and TableUtils in thejsapi-utils package.
  • Update dashboard-core-plugins, code-studio, embed-grid` to match the IrisGrid package changes.

BREAKING CHANGE:

  • DateUtils static methods makeDateWrapper, getNextDate , parseDateRange now require the JSAPI object as the first argument.
  • IrisGridUtils static methods dehydrateIrisGridState, hydrateIrisGridState, hydrateQuickFilters, dehydrateAdvancedFilters, hydrateAdvancedFilters, dehydrateAdvancedFilterOptions, hydrateAdvancedFilterOptions, dehydratePendingDataMap, hydratePendingDataMap, dehydrateValue, hydrateValue, dehydrateDateTime, hydrateDateTime, hydrateLong, hydrateSort, applyTableSettings, getFiltersFromInputFilters, rangeSetFromRanges converted to instance methods. Consumers now need to create an IrisGridUtils instance and pass the JSAPI object to the constructor.
  • TableUtils static methods makeQuickFilter, makeQuickFilterFromComponent, makeQuickNumberFilter, makeQuickTextFilter, makeQuickBooleanFilter, makeQuickDateFilter, makeQuickDateFilterWithOperation, makeQuickCharFilter, makeAdvancedFilter, makeAdvancedValueFilter, makeFilterValue, makeFilterRawValue, makeValue, makeSelectValueFilter converted to instance methods. Consumers now need to create a TableUtils instance and pass the JSAPI object to the constructor.
  • IrisGridTableModel, IrisGridTableModelTemplate, IrisGridProxyModel constructors require the JSAPI object in the first argument.
  • IrisGridTestUtils.makeModel, IrisGridModelFactory.makeModel now require the JSAPI object in the first argument.
  • IrisGridContextMenuHandler constructor requires the JSAPI object in the second argument.
  • IrisGridPanel requires a new makeApi prop, a function that resolves with the JSAPI instance.
  • CrossColumnSearch.createSearchFilter requires the JSAPI object argument.
  • Components AdvancedFilterCreatorSelectValue, AdvancedFilterCreatorSelectValueList, ChartBuilder, GotoRow, IrisGrid, IrisGridModelUpdater, IrisGridPartitionSelector, PartitionSelectorSearch, TableCSVExporter, TableSaver, TreeTableViewportUpdater, RowFormatEditor, ColumnFormatEditor, ConditionEditor now require the JSAPI object passed in the new prop dh.
  • Components AdvancedFilterCreator, AdvancedFilterCreatorFilterItem require the TableUtils instance pass in the new prop tableUtils.
  • ConditionalFormattingUtils static methods getFormatColumns, isDateConditionValid require the JSAPI object in the first argument.
  • ConditionalFormattingAPIUtils static method makeRowFormatColumn requires the JSAPI object in the first argument.

@vbabich vbabich self-assigned this May 4, 2023
@vbabich vbabich requested a review from mofojed May 4, 2023 12:57
@vbabich vbabich marked this pull request as ready for review May 4, 2023 12:58
Copy link
Copy Markdown
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

Looks good overall, just a little suggested cleanup.

Comment thread packages/dashboard-core-plugins/src/panels/IrisGridPanel.tsx Outdated
>;
export interface IrisGridProps {
children: React.ReactNode;
dh: DhType;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Side note - it would be ideal if everything was abstracted through the model and we didn't have to depend on dh being passed in to IrisGrid. I think that change is much more work than we're looking for right now though with this change.

Also, we could still use the context in the class component to get the API from the context: https://legacy.reactjs.org/docs/context.html#classcontexttype

I don't think that's much better though.

Comment thread packages/iris-grid/src/mousehandlers/IrisGridColumnSelectMouseHandler.ts Outdated
Comment thread packages/iris-grid/src/sidebar/aggregations/AggregationEdit.tsx Outdated
@vbabich vbabich force-pushed the jsapi-deglobalize-irisgrid branch from aff5958 to 72fbc81 Compare May 8, 2023 13:28
@codecov
Copy link
Copy Markdown

codecov Bot commented May 8, 2023

Codecov Report

Merging #1262 (5128dbf) into main (87fa2ef) will increase coverage by 0.03%.
The diff coverage is 65.00%.

@@            Coverage Diff             @@
##             main    #1262      +/-   ##
==========================================
+ Coverage   45.39%   45.43%   +0.03%     
==========================================
  Files         502      502              
  Lines       34706    34764      +58     
  Branches     8677     8679       +2     
==========================================
+ Hits        15754    15794      +40     
- Misses      18901    18919      +18     
  Partials       51       51              
Flag Coverage Δ
unit 45.43% <65.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/code-studio/src/main/AppMainContainer.tsx 34.35% <0.00%> (-0.12%) ⬇️
packages/code-studio/src/main/WidgetUtils.ts 5.55% <0.00%> (ø)
packages/dashboard-core-plugins/src/GridPlugin.tsx 48.00% <0.00%> (-4.18%) ⬇️
...ckages/dashboard-core-plugins/src/PandasPlugin.tsx 50.00% <0.00%> (-2.39%) ⬇️
packages/iris-grid/src/ColumnStatistics.tsx 3.40% <ø> (ø)
packages/iris-grid/src/CrossColumnSearch.tsx 8.64% <ø> (ø)
packages/iris-grid/src/GotoRow.tsx 39.58% <ø> (ø)
packages/iris-grid/src/IrisGridCopyHandler.tsx 82.80% <ø> (ø)
packages/iris-grid/src/IrisGridMetricCalculator.ts 70.58% <ø> (ø)
packages/iris-grid/src/IrisGridModel.ts 31.66% <ø> (ø)
... and 41 more

@vbabich vbabich force-pushed the jsapi-deglobalize-irisgrid branch from 72fbc81 to f1f7f75 Compare May 8, 2023 15:34
Copy link
Copy Markdown
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

Looks good, need to update the Breaking Change bit

Slightly confused how text in the Go To Row has gotten shifted down by a pixel

@vbabich
Copy link
Copy Markdown
Collaborator Author

vbabich commented May 8, 2023

Updated breaking changes in the first comment.
The 1px shift in the Go To Row doesn't seem to be related, I'm getting the same e2e result on latest main:
117 pixels (ratio 0.01 of all image pixels) are different. I guess no one updated the screenshot in a while.

@vbabich vbabich merged commit 588cb8f into deephaven:main May 8, 2023
@github-actions github-actions Bot locked and limited conversation to collaborators May 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants