-
Notifications
You must be signed in to change notification settings - Fork 34
refactor: improve proposal dashboard #1926
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
refactor: improve proposal dashboard #1926
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 @Junjiequan - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/app/proposals/proposal-table/proposal-table.component.ts:264` </location>
<code_context>
});
}
- onGlobalTextSearchChange(text: string) {
- this.router.navigate([], {
- queryParams: {
- textSearch: text || undefined,
- pageIndex: 0,
- },
- queryParamsHandling: "merge",
- });
- }
</code_context>
<issue_to_address>
onGlobalTextSearchChange is defined but not used.
If unused, please remove this method to keep the codebase clean.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
onGlobalTextSearchChange(text: string) {
this.router.navigate([], {
queryParams: {
textSearch: text || undefined,
pageIndex: 0,
},
queryParamsHandling: "merge",
});
}
=======
>>>>>>> REPLACE
</suggested_fix>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
a661412
to
71653ad
Compare
…ponent and ProposalSearchBarComponent add shared-filter component and shared full-text-search-bar component replace count with fullfacet
65b5aa2
to
669f115
Compare
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.
Can we add the following fields to the list of columns in the proposals table?
- Type
- Start Date
- End Date
Other than that it looks good
Also, can we take care of the conflicts that are mentioned here? |
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.
It looks good to me in general. A small thing that I found hard to figure out is what Start Time
and End Time
filters really filter by. There are no such fields on the proposal as I can see.
src/app/proposals/proposal-filters/side-bar-filter/proposal-side-filter.component.html
Show resolved
Hide resolved
src/app/proposals/proposal-table/proposal-table.component.spec.ts
Outdated
Show resolved
Hide resolved
@martin-trajanovski startTime and endTime are optional fields, they are defined in the proposal schema |
Description
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
Refactor proposal dashboard to use a shared full-text search bar component and extract table functionality into a dedicated ProposalTableComponent, streamlining search and table logic across dataset and proposal dashboards.
Enhancements:
Tests: