-
Notifications
You must be signed in to change notification settings - Fork 1
Month selector on forecast page #729
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
Conversation
astride
left a 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.
Litt pirk og noen forslag 😄 Skal teste ut lokalt nå!
| searchParams.startDate ? `Date=${searchParams.startDate}` : "" | ||
| }${ | ||
| searchParams.monthCount | ||
| ? `&MonthCount=${searchParams.monthCount}` |
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.
Kan vi ende opp i en situasjon hvor searchParams.monthCount har verdi, men searchParams.startDate ikke har det? Med tanke på bruken av ? og &
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.
Tror jeg har skrevet de slik at de ikke henger sammen? Men kan være jeg ser feil 😅
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.
Neida, jeg er enig i den! Det jeg tenkte på var at URL-en blir .../forecasts?&MonthCount=..., hvis jeg ser riktig? Er det greit med ?&? Godt mulig at det er det, altså 😄 Jeg bare så for meg at det burde ha vært kun ? i det tilfellet.
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.
Ahh, da skjønner jeg hva du mener! Skal teste litt! Tror det går fint, men skal være mulig å kun få det om man bare har en av de :D
| const [date, setDate] = useState(searchParams.get("startDate") || ""); | ||
| const quarterSpan = Number.parseInt(searchParams.get("quarterSpan") ?? "4"); | ||
| const [monthCount, setMonthCount] = useState( | ||
| Number.parseInt(searchParams.get("monthCount") ?? "12"), |
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.
Vil foreslå å lage en default month count-konstant og bruke den her og for defaultFilters 😄
| dropDownFunction={getNumberFromMonthSpan} | ||
| /> | ||
| <ActionButton variant="secondary" onClick={resetSelectedMonth}> | ||
| Nåværende måned |
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.
Dette er kanskje noe vi bør høre med Odd Morten om: Slik det er i backend nå, så justeres startmåneden sånn at man alltid ser hele kvartaler om gangen (etter ønske fra Odd Morten da Prognose-siden ble implementert).
Skulle vi enten ha justert teksten som vises her for å gjenspeile det (f.eks. Nåværende kvartal), eller ha endret logikk i backend slik at man ikke lenger er knyttet til kvartal?
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.
Tilsvarende spørsmål for venstre- og høyre-knappene under
[EDIT: Ser at du har tatt høyde for det i logikken i useSelectedMonth 👌]
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.
Tror jeg foretrekker "nåværende kvartal"! Kan endre slik at det står "kvartal" i stedet for måned, i koden og, siden det er slik den fungerer
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.
Ja, nice! 👌
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.
Oo, kom på en ting til. Burde month: 4 i useSelectedMonth ha vært month: 3? For å flytte ett kvartal om gangen?
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.
Det burde det helt klart være!
|
|
||
| function changeSelectedMonth(increase: boolean) { | ||
| const date = startDate ? DateTime.fromISO(startDate) : DateTime.now(); | ||
| //finds thursday in week. allways correct year |
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.
Kommentar som henger igjen? 🤠
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.
Ja, hang igjen fra at jeg kopierte fra useSelectedWeek 😅
| <ActiveFilters context={FilteredForecastContext} /> | ||
| </div> | ||
| </div> | ||
| <MonthSelector /> |
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.
|
Close in favour of #732 |

No description provided.