Skip to content
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

Group partners by age #2685

Merged
merged 5 commits into from
Dec 9, 2024
Merged

Group partners by age #2685

merged 5 commits into from
Dec 9, 2024

Conversation

RoyEJohnson
Copy link
Contributor

@RoyEJohnson RoyEJohnson commented Nov 25, 2024

src/app/models/salesforce-partners.js Show resolved Hide resolved
src/app/pages/partners/partners.js Show resolved Hide resolved
@@ -0,0 +1,67 @@
import React from 'react';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The functions didn't change much, but I extracted the Dialog to its own module, I moved the instance of the Dialog up a level to the Results, and of course added types. A side-by-side compare to the deleted result-grid.js would be useful.

@@ -0,0 +1,300 @@
import React from 'react';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again a side-by-side with results.js could be helpful.

Copy link
Member

Choose a reason for hiding this comment

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

git mv would be nice for reviewing these

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was git mvd in the first commit, but the number of changes made git treat it as delete and create.

);
const equityOptionValues = equityOptions.map((entry) => entry.value);

type PartnerData = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Big new type

};
}

type Ages = '10' | '7' | '4' | '1' | 'new';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the new groupings

};
}

function SeeMore({defaultOpen, children}) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer using this


type Ages = '10' | '7' | '4' | '1' | 'new';

function ResultGridLoader({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was revamped to create the age groupings rather than the old groupings.

import {useNavigate, useLocation} from 'react-router-dom';
import type {LinkTexts, PartnerEntry} from './results';

export default function SelectedPartnerDialog({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extracted these routines from result-grid, where they didn't really belong.

@@ -41,7 +41,7 @@ describe('details/common/hooks', () => {

render(<Component />);

await screen.findByText('28,29,31,32,39,41,42,48,55,56,61,7,70,79,9');
await screen.findByText('1938,1951,1955,1962,1984,2109');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to update some test data files because the structures were out of date, so I have some new results for this test.

@RoyEJohnson RoyEJohnson requested a review from jivey November 25, 2024 20:38
rating={summary.rating}
count={summary.count}
showNumber />
*/}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe can remove this

@@ -0,0 +1,300 @@
import React from 'react';
Copy link
Member

Choose a reason for hiding this comment

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

git mv would be nice for reviewing these

@@ -1,7 +1,7 @@
import {useState, useEffect} from 'react';
import buildContext from '~/components/jsx-helpers/build-context';

function useContextValue(partner) {
function useContextValue(partner: unknown) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this using the PartnerData type and could be annotated with it? I'm not sure how this get used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dialog-context provides info about the selected Partner for the Dialog, but it looks like there's not really a reason for that layer now. I'll get rid of it (or will run into the reason I can't get rid of it).

src/app/pages/partners/results/results.tsx Show resolved Hide resolved
@RoyEJohnson RoyEJohnson force-pushed the group-partners-by-age branch from 02e157f to b9df3aa Compare December 3, 2024 16:35
@RoyEJohnson RoyEJohnson requested a review from jivey December 3, 2024 17:15
Remove dialog-context, pass title/setTitle around
Remove commented-out stars and count
Stop re-shuffling partners every time you view details
Use Object.keys instead of Reflect.ownKeys
@RoyEJohnson RoyEJohnson force-pushed the group-partners-by-age branch from b9df3aa to 9e2bd1f Compare December 3, 2024 21:39
@RoyEJohnson RoyEJohnson merged commit 7b7e836 into main Dec 9, 2024
1 check passed
@RoyEJohnson RoyEJohnson deleted the group-partners-by-age branch December 9, 2024 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants