-
Notifications
You must be signed in to change notification settings - Fork 44
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
Circulars Archive Group View #2617
base: main
Are you sure you want to change the base?
Changes from all commits
1b11e53
22aa2be
8bb3902
c86e6fb
63465c1
325f1f8
772831d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,24 +16,28 @@ export default function PaginationSelectionFooter({ | |
limit, | ||
query, | ||
form, | ||
view, | ||
}: { | ||
page: number | ||
totalPages: number | ||
limit?: number | ||
query?: string | ||
form: string | ||
view?: string | ||
}) { | ||
const submit = useSubmit() | ||
|
||
return ( | ||
<div className="display-flex flex-row flex-wrap"> | ||
<div className="display-flex flex-align-self-center margin-right-2 width-auto"> | ||
<div> | ||
<input type="hidden" form={form} name="view" id="view" value={view} /> | ||
<Select | ||
id="limit" | ||
title="Number of results per page" | ||
className="width-auto height-5 padding-y-0 margin-y-0" | ||
name="limit" | ||
defaultValue="100" | ||
value={limit} | ||
form={form} | ||
onChange={({ target: { form } }) => { | ||
submit(form) | ||
|
@@ -53,6 +57,7 @@ export default function PaginationSelectionFooter({ | |
page={page} | ||
limit={limit} | ||
totalPages={totalPages} | ||
view={view} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does the Pagination component need to know about the view? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. because in getPageLink if the view is not set as a search param, then when it creates the link, it doesn't include view. If it doesn't include view, then the view defaults to index. If it's always index, then the pagination links will never work for groups. To show you what I mean, here is what happens when I remove the view from getPageLinks. When I hover over the 2 button, this is the link. You will see that it doesn't include the view so when that page is navigated to, it's the index view which is the default. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see. The last time that I looked at this component, I noticed that a lot of apparently separate concerns were leaking into it from pages that use it. At some point, I'd like to come back to this and try to refactor it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that's a good idea. It did feel odd to have to add view specific code to it. |
||
/> | ||
)} | ||
</div> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
/*! | ||
* Copyright © 2023 United States Government as represented by the | ||
* Administrator of the National Aeronautics and Space Administration. | ||
* All Rights Reserved. | ||
* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
import { Link } from '@remix-run/react' | ||
|
||
import type { SynonymGroupWithMembers } from '../synonyms/synonyms.lib' | ||
|
||
export default function ({ | ||
allItems, | ||
totalItems, | ||
searchString, | ||
query, | ||
}: { | ||
allItems: SynonymGroupWithMembers[] | ||
totalItems: number | ||
searchString: string | ||
query?: string | ||
}) { | ||
return ( | ||
<> | ||
{query && ( | ||
<h3> | ||
{totalItems} result{totalItems != 1 && 's'} found. | ||
</h3> | ||
)} | ||
|
||
{allItems.map(({ group, members }) => ( | ||
<div key={group.synonymId}> | ||
<details> | ||
<summary> | ||
<Link to={`/circulars/group/${group.synonymId}${searchString}`}> | ||
{group.eventIds.join(', ')} | ||
</Link> | ||
</summary> | ||
<ol className="margin-left-3"> | ||
{members.map(({ circularId, subject }) => { | ||
return ( | ||
<li key={circularId} value={circularId}> | ||
<Link to={`/circulars/${circularId}`}>{subject}</Link> | ||
</li> | ||
) | ||
})} | ||
</ol> | ||
</details> | ||
</div> | ||
))} | ||
</> | ||
) | ||
} |
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.
Why was this addition necessary?
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.
when I moved the component into the components directory, it is judged as a component definition and causes this warning:
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.
I was trying to make the move have no changes to the code if possible