-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: Dynamic most recent date filter #1291
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
95d31ec
to
058f82d
Compare
058f82d
to
323ce96
Compare
…into feat/most-recent-date-filter
…into feat/most-recent-date-filter
@bprusinowski Could we adjust a little bit on the margins and paddings: https://www.figma.com/file/Hn9wvuEYLUmdJtg4KwZYUx/visualize.admin.ch---design?type=design&node-id=2151-136045&mode=design&t=eufhsDZ6QmDsMILm-4 |
Sure, will do! :) |
value: `${value.value}`, | ||
}; | ||
} | ||
} |
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.
Not sure I understand this 😓 Can you give me an example, or add a test 😁 ?
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'll add a test, but just to explain: possible filters coming from the backend do not return VISUALIZE_MAX_VALUE
, but rather an actual date (e.g. 2023), so if we didn't replace it back with VISUALIZE_MAX_VALUE
, we would potentially trigger an update that shouldn't happen (as in fact, the pinned date didn't change, but is still equal to VISUALIZE_MAX_VALUE
). Maybe it would make sense to already do this replacement on the backend side? 🤔
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.
LGTM 👏 One unit test on buildFilters could be good ?
<MUISwitch | ||
checked={usesMostRecentDate} | ||
onChange={() => | ||
fieldProps.onChange({ |
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.
Nit: Instead of relying on the state usesMOstRecentDate, I think it is preferrable to rely on the checked
value coming in the 2nd argument of the callback. This way the function could be stable, and I think it'd be clearer since here you have to think "ok usesMostRecentDate is true, so when it is clicked it shuold be false, and the max date should be used". IN the callback, the checked parameter is already right, so I think it's clearer.
sparqlClient.query, | ||
{ operation: "postUrlencoded" } | ||
); | ||
const maxValue = maxValueRaw?.[0]?.value?.value as string; |
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.
Function could be extracted for clarity ? It could aso be memoized ?
It made me think that if we'd use react-query, we could cache the sparql client results. Then I remember that we are using urql for it's graphql abilities 🤷
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.
Yup, it can be extracted 👍 But for memoization I am not sure how would you memoize it, I guess when it's extracted it's already "memoized"? (as we are not in the useMemo world here in the backend 😅)
Thanks for the review @ptbrowne 🙇♂️ I'll try to tackle the changes this week, otherwise I'll make a note to myself to do it in the new year 😅 |
This PR:
TODO
Context
For the description of the technical approach see this Notion document.
How to test