-
Notifications
You must be signed in to change notification settings - Fork 5
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
add homeTable skeleton #106
Conversation
ea89fe9
to
59c3f51
Compare
6bd289d
to
d1b4671
Compare
7202e46
to
ace7c1d
Compare
src/i18n-en.js
Outdated
readQuestionnaire: 'Read questionnaire', | ||
close: 'Close', | ||
comment: 'Comment', | ||
surveyUnitNumber: 'SU n°', |
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.
maybe Su n.
src/i18n-fr.js
Outdated
subSample: 'Sous-éch.', | ||
interviewer: 'Enquêteur', | ||
state: 'Etat', | ||
closingCause: 'Bilan agrégé', |
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.
Motif de clôture
src/i18n-en.js
Outdated
@@ -7,7 +7,7 @@ export const messagesEn = { | |||
goToReassignment: 'Reassignment', | |||
goToHelp: 'HELP', | |||
logout: 'Logout', | |||
selectFavoriteSurveys: 'Select my favorite surveys', | |||
selectFavoriteSurveys: 'MY FAVORITE SURVEYS', |
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.
Could keep standard case in i18n and convert it to uppercase in component
src/i18n-fr.js
Outdated
@@ -7,7 +7,7 @@ export const messagesFr = { | |||
goToReassignment: 'Réaffectation', | |||
goToHelp: 'AIDE', | |||
logout: 'Se déconnecter', | |||
selectFavoriteSurveys: 'Sélectionner mes enquêtes favorites', | |||
selectFavoriteSurveys: 'MES ENQUÊTES FAVORITES', |
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.
same case here 🤭
src/routes.tsx
Outdated
@@ -26,6 +27,7 @@ export const routes: RouteObject[] = [ | |||
{ path: "notify", element: <NotifyPage /> }, | |||
{ path: "collectOrganization", element: <CollectOrganizationPage /> }, | |||
{ path: "reassignment", element: <ReassignmentPage /> }, | |||
{ path: "surveyUnits/:id", element: <SurveyUnitPage /> }, |
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.
shouldn't the path be /survey-unit/{id}
url should be case insensitive => kebab case rulz ✊
src/ui/CommentDialog.tsx
Outdated
}; | ||
|
||
const handleDelete = () => { | ||
// TODO call api to delete 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.
thinking :
create an api interface layer and call its methods in components
could mock api interface for tests and remove // TODOs in component 😁
src/ui/CommentDialog.tsx
Outdated
InputProps={{ | ||
startAdornment: <InputAdornment position="start" />, | ||
}} | ||
autoFocus |
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.
accessibility is not fond of autofocus
to check with a11y referent
<TableCell sx={{ typography: "itemSmall" }}>{surveyUnit.ssech ?? "-"}</TableCell> | ||
<TableCell sx={{ typography: "itemSmall" }}>{surveyUnit.interviewer ?? "-"}</TableCell> | ||
<TableCell sx={{ typography: "itemSmall" }}> | ||
<StateChip value={surveyUnit.states} /> |
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.
UX really want to show StateEnum entry value VIC
, AOC
, WFT
, CLO
, [...] ?
=> probably need a translation like what is done with surveyUnit.closingCause NPX
, NPA
, [...]
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.
translation is done by StateChip component ;)
src/ui/TableHeadCell.test.tsx
Outdated
|
||
describe("TableHeadCell component", () => { | ||
it("should render label, sort icon and call sort function ", () => { | ||
const onRequestSortMock = vi.fn(); |
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.
if onRequestSort is always mocked, it could be define once for all it
blocks
src/ui/TableHeadCell.tsx
Outdated
if (!onRequestSort) { | ||
return; | ||
} | ||
onRequestSort(event, property); |
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.
if (!onRequestSort) { | |
return; | |
} | |
onRequestSort(event, property); | |
onRequestSort?.(event, property); |
optional chaining to ensure that the function is not null or undefined
b3020f9
to
7baff14
Compare
|
No description provided.