-
Notifications
You must be signed in to change notification settings - Fork 35
refactor(dataset): redesign & relocate data buttons #2169
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 1 issue, and left some high level feedback:
- The
currentPublicViewModestate is still mixing''andbooleanvalues (and now treats both''andfalseas the same in the template); consider normalizing this to a single, explicit type (e.g. an enum or strict boolean) and updating the selector/usage accordingly to avoid brittle comparisons. - In
onViewPublicChangeyou’re now directly dispatchingfetchDatasetsActionandfetchFacetCountsActionfrom the component; if these are intended side effects ofsetPublicViewModeAction, consider moving them into an effect so that components only express state changes and not the associated data-loading logic. - In
datasets-filter.component.html, theng-template [ngIf]="this.loggedIn$ | async"is a bit unusual for Angular templates; using a more conventional*ngIf="loggedIn$ | async"on anng-container(and droppingthis.) would simplify the template and improve readability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `currentPublicViewMode` state is still mixing `''` and `boolean` values (and now treats both `''` and `false` as the same in the template); consider normalizing this to a single, explicit type (e.g. an enum or strict boolean) and updating the selector/usage accordingly to avoid brittle comparisons.
- In `onViewPublicChange` you’re now directly dispatching `fetchDatasetsAction` and `fetchFacetCountsAction` from the component; if these are intended side effects of `setPublicViewModeAction`, consider moving them into an effect so that components only express state changes and not the associated data-loading logic.
- In `datasets-filter.component.html`, the `ng-template [ngIf]="this.loggedIn$ | async"` is a bit unusual for Angular templates; using a more conventional `*ngIf="loggedIn$ | async"` on an `ng-container` (and dropping `this.`) would simplify the template and improve readability.
## Individual Comments
### Comment 1
<location> `src/app/datasets/datasets-filter/datasets-filter.component.ts:96` </location>
<code_context>
];
searchPublicDataEnabled = this.appConfig.searchPublicDataEnabled;
- currentPublicViewMode: boolean | "" = "";
subscriptions: Subscription[] = [];
</code_context>
<issue_to_address>
**issue (complexity):** Consider removing the duplicated `currentPublicViewMode` state, binding directly to the store observable, and optionally moving the follow-up dispatches into an effect to simplify the component.
You can trim the new complexity without changing behavior by (1) removing duplicated state and (2) tightening the types. The orchestration can also move to an effect if you’re open to that.
### 1. Remove `currentPublicViewMode` and the manual subscription
You don’t need a local copy if the store is the source of truth. Expose it as an observable and bind via `async` in the template.
**Component:**
```ts
// remove this:
// currentPublicViewMode: boolean | "" = "";
// add this instead:
publicViewMode$ = this.store.select(selectPublicViewMode);
```
```ts
ngOnInit(): void {
// remove this whole block:
// this.subscriptions.push(
// this.store.select(selectPublicViewMode).subscribe((publicViewMode) => {
// this.currentPublicViewMode = publicViewMode;
// }),
// );
}
```
**Template usage (example):**
```html
<!-- before (example) -->
<!-- <toggle [checked]="currentPublicViewMode" (change)="onViewPublicChange($event)"></toggle> -->
<!-- after -->
<toggle
[checked]="publicViewMode$ | async"
(change)="onViewPublicChange($event)"
></toggle>
```
### 2. Simplify the handler and typing
You no longer need `currentPublicViewMode` in the handler—use the event value directly. This also removes the `boolean | ""` union.
```ts
onViewPublicChange(isPublished: boolean) {
this.store.dispatch(setPublicViewModeAction({ isPublished }));
this.store.dispatch(fetchDatasetsAction());
this.store.dispatch(fetchFacetCountsAction());
}
```
This keeps all behaviors (including the two fetches) but makes the flow obvious and type-safe.
### 3. (Optional) Move orchestration into an effect
If you want the component even leaner, you can push the extra dispatches into NgRx effects:
```ts
// datasets.effects.ts
viewModeChanged$ = createEffect(() =>
this.actions$.pipe(
ofType(setPublicViewModeAction),
switchMap(() => [
fetchDatasetsAction(),
fetchFacetCountsAction(),
]),
),
);
```
```ts
// component
onViewPublicChange(isPublished: boolean) {
this.store.dispatch(setPublicViewModeAction({ isPublished }));
}
```
This keeps the same behavior while centralizing orchestration logic and reducing component complexity.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Don't have a strong opinion on where this should be placed but:
Imo this is a bug in the backend and the frontend should explicitly filter for what is displayed instead of relying on the backend behavior:
|
Junjiequan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description
This PR moves the data buttons from top of the table to the filter section.
Before:

After:
Motivation
Background on use case, changes needed
Fixes:
Please provide a list of the fixes implemented in this PR
Changes:
Please provide a list of the changes implemented by this PR
Tests included
Documentation
official documentation info
If you have updated the official documentation, please provide PR # and URL of the pages where the updates are included
Backend version
Summary by Sourcery
Relocate and restyle the datasets public/embargoed data toggle from the table actions toolbar into the filter panel and wire it to the datasets filtering store state.
New Features:
Enhancements: