Skip to content

feat: Make rollup group behaviour a setting in the global settings menu #2183

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.

Sign up for GitHub

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

Merged
AkshatJawne merged 27 commits into from
Sep 3, 2024
Merged

feat: Make rollup group behaviour a setting in the global settings menu #2183

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
fd14237
WIP
AkshatJawne Aug 8, 2024
3941b7c
WIP
AkshatJawne Aug 12, 2024
60a585f
WIP
AkshatJawne Aug 12, 2024
7a5d4e2
WIP
AkshatJawne Aug 12, 2024
a25bd9a
modify Updater
AkshatJawne Aug 13, 2024
fd20dd0
modify test case
AkshatJawne Aug 14, 2024
55c315a
modify logic
AkshatJawne Aug 15, 2024
4092e79
make review changes
AkshatJawne Aug 15, 2024
d5ebdd0
remove unneccessary import
AkshatJawne Aug 15, 2024
32faad6
Merge branch 'main' into 2128_rollup_settings
AkshatJawne Aug 19, 2024
e98abba
add snapshots
AkshatJawne Aug 19, 2024
b8f3213
make change to object
AkshatJawne Aug 19, 2024
8c80230
make changes according to review
AkshatJawne Aug 21, 2024
1cbc60d
address PR comments
AkshatJawne Aug 22, 2024
f0ca4ab
adjust formatter test
AkshatJawne Aug 22, 2024
7d8fc95
add exceptions
AkshatJawne Aug 22, 2024
68a4148
remove exceptions
AkshatJawne Aug 22, 2024
7d8f09e
add back deps
AkshatJawne Aug 22, 2024
bb7c663
make changes based on comment
AkshatJawne Aug 22, 2024
024db4f
Merge branch 'main' into 2128_rollup_settings
AkshatJawne Aug 22, 2024
ca8f90b
update snapshots
AkshatJawne Aug 23, 2024
a1bd2eb
resolve conflicts
AkshatJawne Aug 23, 2024
2fa7306
readd formatter check and update snapshots
AkshatJawne Aug 23, 2024
fa517e3
update check and snapshots
AkshatJawne Aug 23, 2024
48d061d
get snapshots from main
AkshatJawne Aug 23, 2024
f10645a
alter test case
AkshatJawne Aug 23, 2024
06d3433
fix test case
AkshatJawne Sep 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/app-utils/src/storage/LocalWorkspaceStorage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ export class LocalWorkspaceStorage implements WorkspaceStorage {
truncateNumbersWithPound: false,
showEmptyStrings: true,
showNullStrings: true,
showExtraGroupColumn: true,
defaultNotebookSettings: {
isMinimapEnabled: false,
},
Expand Down Expand Up @@ -103,6 +104,10 @@ export class LocalWorkspaceStorage implements WorkspaceStorage {
serverConfigValues,
'showNullStrings'
),
showExtraGroupColumn: LocalWorkspaceStorage.getBooleanServerConfig(
serverConfigValues,
'showExtraGroupColumn'
),
defaultNotebookSettings:
serverConfigValues?.get('isMinimapEnabled') !== undefined
? {
Expand Down
35 changes: 35 additions & 0 deletions packages/code-studio/src/settings/FormattingSectionContent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
getTruncateNumbersWithPound,
getShowEmptyStrings,
getShowNullStrings,
getShowExtraGroupColumn,
updateSettings as updateSettingsAction,
RootState,
WorkspaceSettings,
Expand Down Expand Up @@ -56,6 +57,7 @@ interface FormattingSectionContentProps {
truncateNumbersWithPound: boolean;
showEmptyStrings: boolean;
showNullStrings: boolean;
showExtraGroupColumn: boolean;
updateSettings: (settings: Partial<WorkspaceSettings>) => void;
defaultDecimalFormatOptions: FormatOption;
defaultIntegerFormatOptions: FormatOption;
Expand All @@ -72,6 +74,7 @@ interface FormattingSectionContentState {
truncateNumbersWithPound: boolean;
showEmptyStrings: boolean;
showNullStrings: boolean;
showExtraGroupColumn: boolean;
timestampAtMenuOpen: Date;
}

Expand Down Expand Up @@ -113,6 +116,8 @@ export class FormattingSectionContent extends PureComponent<
this.handleShowEmptyStringsChange.bind(this);
this.handleShowNullStringsChange =
this.handleShowNullStringsChange.bind(this);
this.handleShowExtraGroupColumnChange =
this.handleShowExtraGroupColumnChange.bind(this);

const {
defaultDateTimeFormat,
Expand All @@ -124,6 +129,7 @@ export class FormattingSectionContent extends PureComponent<
truncateNumbersWithPound,
showEmptyStrings,
showNullStrings,
showExtraGroupColumn,
} = props;

this.containerRef = React.createRef();
Expand All @@ -139,6 +145,7 @@ export class FormattingSectionContent extends PureComponent<
truncateNumbersWithPound,
showEmptyStrings,
showNullStrings,
showExtraGroupColumn,
timestampAtMenuOpen: new Date(),
};
}
Expand Down Expand Up @@ -330,6 +337,15 @@ export class FormattingSectionContent extends PureComponent<
this.queueUpdate(update);
}

handleShowExtraGroupColumnChange(): void {
const { showExtraGroupColumn } = this.state;
const update = {
showExtraGroupColumn: !showExtraGroupColumn,
};
this.setState(update);
this.queueUpdate(update);
}

commitChanges(): void {
const { updateSettings } = this.props;
const updates = this.pendingUpdates.reduce(
Expand All @@ -356,6 +372,7 @@ export class FormattingSectionContent extends PureComponent<
truncateNumbersWithPound,
showEmptyStrings,
showNullStrings,
showExtraGroupColumn,
} = this.state;

const {
Expand Down Expand Up @@ -596,6 +613,23 @@ export class FormattingSectionContent extends PureComponent<
</Checkbox>
</div>
</div>

<div className="form-row mb-3">
<label
className="col-form-label col-3"
htmlFor="default-integer-format-input"
Comment thread
AkshatJawne marked this conversation as resolved.
Outdated
>
Rollup
</label>
<div className="col pr-0 pt-2">
<Checkbox
checked={showExtraGroupColumn ?? null}
Comment thread
AkshatJawne marked this conversation as resolved.
Outdated
onChange={this.handleShowExtraGroupColumnChange}
>
Show extra &quot;group&quot; column
</Checkbox>
</div>
</div>
</div>
</div>
);
Expand All @@ -614,6 +648,7 @@ const mapStateToProps = (
truncateNumbersWithPound: getTruncateNumbersWithPound(state),
showEmptyStrings: getShowEmptyStrings(state),
showNullStrings: getShowNullStrings(state),
showExtraGroupColumn: getShowExtraGroupColumn(state),
timeZone: getTimeZone(state),
defaults: getDefaultSettings(state),
});
Expand Down
14 changes: 12 additions & 2 deletions packages/iris-grid/src/IrisGrid.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,7 @@ class IrisGrid extends Component<IrisGridProps, IrisGridState> {
truncateNumbersWithPound: false,
showEmptyStrings: true,
showNullStrings: true,
showExtraGroupColumn: true,
formatter: EMPTY_ARRAY,
decimalFormatOptions: PropTypes.shape({
defaultFormatString: PropTypes.string,
Expand Down Expand Up @@ -637,6 +638,7 @@ class IrisGrid extends Component<IrisGridProps, IrisGridState> {
this.truncateNumbersWithPound = false;
this.showEmptyStrings = true;
this.showNullStrings = true;
this.showExtraGroupColumn = true;

// When the loading scrim started/when it should extend to the end of the screen.
this.tableSaver = null;
Expand Down Expand Up @@ -1023,6 +1025,8 @@ class IrisGrid extends Component<IrisGridProps, IrisGridState> {

showNullStrings: boolean;

showExtraGroupColumn: boolean;

// When the loading scrim started/when it should extend to the end of the screen.
loadingScrimStartTime?: number;

Expand Down Expand Up @@ -1870,6 +1874,7 @@ class IrisGrid extends Component<IrisGridProps, IrisGridState> {

const showEmptyStrings = settings?.showEmptyStrings ?? true;
const showNullStrings = settings?.showNullStrings ?? true;
const showExtraGroupColumn = settings?.showExtraGroupColumn ?? true;

const isColumnFormatChanged = !deepEqual(
this.globalColumnFormats,
Expand All @@ -1892,6 +1897,8 @@ class IrisGrid extends Component<IrisGridProps, IrisGridState> {
const isShowEmptyStringsChanged =
this.showEmptyStrings !== showEmptyStrings;
const isShowNullStringsChanged = this.showNullStrings !== showNullStrings;
const isShowExtraGroupColumnChanged =
this.showExtraGroupColumn !== showExtraGroupColumn;

if (
isColumnFormatChanged ||
Expand All @@ -1900,7 +1907,8 @@ class IrisGrid extends Component<IrisGridProps, IrisGridState> {
isIntegerFormattingChanged ||
isTruncateNumbersChanged ||
isShowEmptyStringsChanged ||
isShowNullStringsChanged
isShowNullStringsChanged ||
isShowExtraGroupColumnChanged
) {
this.globalColumnFormats = globalColumnFormats;
this.dateTimeFormatterOptions = dateTimeFormatterOptions;
Expand All @@ -1909,6 +1917,7 @@ class IrisGrid extends Component<IrisGridProps, IrisGridState> {
this.truncateNumbersWithPound = truncateNumbersWithPound;
this.showEmptyStrings = showEmptyStrings;
this.showNullStrings = showNullStrings;
this.showExtraGroupColumn = showExtraGroupColumn;
this.updateFormatter({}, forceUpdate);

if (isDateFormattingChanged && forceUpdate) {
Expand Down Expand Up @@ -1977,7 +1986,8 @@ class IrisGrid extends Component<IrisGridProps, IrisGridState> {
this.integerFormatOptions,
this.truncateNumbersWithPound,
this.showEmptyStrings,
this.showNullStrings
this.showNullStrings,
this.showExtraGroupColumn
);

log.debug('updateFormatter', this.globalColumnFormats, mergedColumnFormats);
Expand Down
12 changes: 5 additions & 7 deletions packages/iris-grid/src/IrisGridModelUpdater.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ interface IrisGridModelUpdaterProps {
const IrisGridModelUpdater = React.memo(
({
model,
modelColumns,
modelColumns: propModelColumns,
top,
bottom,
left,
Expand All @@ -71,6 +71,10 @@ const IrisGridModelUpdater = React.memo(
columnHeaderGroups,
partitionConfig,
}: IrisGridModelUpdaterProps) => {
if (model.formatter !== formatter) {
Copy link
Copy Markdown
Member

@mofojed mofojed Aug 15, 2024

Choose a reason for hiding this comment

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

I think the "less" hacky way to fix this is to get rid of all the useEffects in here. Those run after the render, when really all we want from this component is to set values when they have changed. And then make sure it checks on every render (i.e. the component shouldn't be memoized at the top), and then we don't need to pass in a modelColumns prop. Instead could make a hook to run immediately when something has changed, e.g.

function useOnChange(callback: () => void, deps: DependencyList): void {
  // Want to call the callback if the deps have changed from the previous time
  const prevDeps = usePrevious(deps);
  if (prevDeps === undefined || !deps.every((dep, i) => dep === prevDeps[i])) {
    callback();
  }
}

Then instead of useEffect in IrisGridModelUpdater, just use useOnChange, and use model.columns instead of modelColumns, get rid of the modelColumns prop, and don't wrap the function in React.memo. I think that will work.

model.formatter = formatter;
}
const modelColumns = model.columns;
const columns = useMemo(
() =>
IrisGridUtils.getModelViewportColumns(
Expand Down Expand Up @@ -108,12 +112,6 @@ const IrisGridModelUpdater = React.memo(
},
[model, sorts, reverseType]
);
useEffect(
function updateFormatter() {
model.formatter = formatter;
},
[model, formatter]
);
useEffect(
function updateCustomColumns() {
if (model.isCustomColumnsAvailable) {
Expand Down
12 changes: 9 additions & 3 deletions packages/iris-grid/src/IrisGridTreeTableModel.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,21 @@ const irisGridTestUtils = new IrisGridTestUtils(dh);

describe('IrisGridTreeTableModel virtual columns', () => {
const expectedVirtualColumn = expect.objectContaining({
name: '__DH_UI_GROUP__',
constituentType: 'string',
description: 'Key column',
displayName: 'Group',
index: -1,
isPartitionColumn: false,
isProxy: true,
isSortable: false,
name: '__DH_UI_GROUP__',
type: 'string',
});
const columns = irisGridTestUtils.makeColumns();

test.each([
[0, columns, columns],
[1, columns, columns],
[1, columns, [expectedVirtualColumn, ...columns]],
[1, columns, [expectedVirtualColumn, ...columns]],
[2, columns, [expectedVirtualColumn, ...columns]],
[columns.length, columns, [expectedVirtualColumn, ...columns]],
])(
Expand Down
Loading
⚡