-
Notifications
You must be signed in to change notification settings - Fork 13
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
Ny teknisk plattform: MDX, Algolia-søk, og innlogging #158
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/variant/handbook/7S2jwhTnSZCJuqvK3krwBjnFR1Up |
Funker nå! |
Det var nok for du manglet department i Azure AD. Ser nesten alle gjør det 😓 Mulig vi enten må bruke bemanning eller oppdatere Azure AD med rett data. |
Fixed |
Jeg får også til å logge inn nå, men får «(none)» etter navnet. Antar jeg ikke har department i Azure. Samtidig kjenner jeg litt på at dette reviewet må lukkes snart - det er mange endringer! Hva tenker dere, @mikaelbr @adriansberg? Skal vi merge og heller rette småfeil som oppdages etterhvert? Om ingen får brukt innlogginga (og vi ikke tenker å gjøre noe med det rett etter merge), så kan kanskje knappen skjules midlertidig? |
Det er godt mulig at dette er første gang jeg gjør pull requests i Github 😅😅 |
@tormodhau Jeg har kun testet innlogging, har ikke gjort en review. 🫣 |
@adriansberg, jeg så gjennom koden tidligere i dag. Det er veldig mye, så noe særlig grundig gjennomgang har jeg ikke gjort. Prøver å finne en knapp for å "Approve", men det er visstnok ikke sånn Github PR funker... eller så er jeg blind. 😄 |
@tormodhau Skal være en grønn knapp øverst på sida, der det står "Add your review". 😄 Jeg kan se over koden nå. |
@@ -101,8 102,10 @@ export default function GeneralLayout({ | |||
isActiveHandbook(category.path, asPath, true), | |||
); | |||
|
|||
const classes = and(style.main, !noSidebar ? style.main__sidebar : undefined); | |||
|
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.
Jeg har lagt merke til at and
brukes mye i kodebasene våre. Er det for å unngå den ekstra avhengigheten classnames innfører?
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.
Glad i økosystem jeg altså, men tenker at for utils som er på en linje så trenger vi ikke flere innslag i lockfiles og package files enn vi har i kildekodelinjer 😄
src/components/department/index.tsx
Outdated
isVisible, | ||
}); | ||
} | ||
return null; |
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.
^ linje 58-71
Her synes jeg nøstingen går litt ut over lesbarheten. Foreslår å gjøre begge if
-ene på toppen til Guard Clauses, så blir strukturen helt "flat"?
currentSearch?: string; | ||
} | ||
// @TODO This should be automatically generated from the tree structure | ||
const metadata = { |
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.
Fortjener denne en GIthub-issue, eller skal det ikke prioriteres med det første?
<p>`[email protected]`</p> | ||
</DepartmentItem> | ||
|
||
<DepartmentItem dep="all"> |
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.
Antar det må være en React.Node som children
her, altså at det ikke kan være MDX?
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 kan være markdown også.
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.
(mener jeg, men kan ta 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.
Sett over det meste, ser greit ut 👏
Dette er kun en teknisk endring for underliggende teknologi for håndboken. Det er ikke store endringer i design, innhold eller struktur. Men det er et steg på vegen i arbeid som foregår. Målet her er først og fremst å ha et søk som gjør at vi kan se hva som blir søkt på slik at vi kan bruke det til innsikt i strukturering. Samtidig foregår det arbeid på omstrukturering for å lettere løfte frem innhold som er relevant. Det er også noe designløft som kommer til å gjøres etterhvert.
Dette skal med andre ord fungere ca som før, men forhåpentligvis litt bedre. Så utnytter vi ikke per nå det maksimale potensiale, men jeg ønsker å merge dette nå slik at det ikke blir enda mer langtlevende branch.
For å muliggjøre neste generasjon av Håndbok i et iterativt løp så er det en del tekniske endringer som gjøres:
MDX, rike komponenter
Søkemotor
Login