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

Legge inn/endre tekster for felles forside #1124

Merged
merged 4 commits into from
Mar 12, 2024
Merged

Conversation

daphneleebeek
Copy link
Contributor

@daphneleebeek daphneleebeek commented Mar 8, 2024

💰 Hva forsøker du å løse i denne PR'en

Skriv 1 eller 2 setninger om hvilken funksjonell endring som blir implementert.
Endrer layout og tekster på forsiden, slik at den kan brukes felles for begge søknadene

🔎️ Er det noe spesielt du ønsker å fremheve?

Er det noe du er bekymret eller usikker på? Beskriv det gjerne her.
Vi flyttet "Vi stoler på deg" inn i bekreftelsesboksen, både med og uten toggle. Utover dette skal det kun være endringer med toggle.

✅ Checklist

Har du husket alle punktene i listen?

  • Jeg har testet mine endringer i henhold til akseptansekriteriene/skissene 🕵️
  • Jeg har testet endringene mine i mobilstørrelse, zoom 200%, skalerer riktig med endret tekststørrelse i browser 📱
  • Jeg har skrevet tester. Hvis du ikke har skrevet tester, beskriv hvorfor under 👇
  • Jeg har fikset en bug, og skrevet regresjonstest for denne
  • Jeg har endret søknadskontrakten og modellversjon i Miljø.ts

Jeg har ikke skrevet tester fordi:

🤷‍♀ ️Hvor er det lurt å starte?

F.eks. commit for commit, alt i ett?

💬 Ønsker du en muntlig gjennomgang?

  • Ja
  • Nei

👀 Screen shots

Har det visuelle endret seg? Legg til før- og etterbilder!

TOGGLE AV:
Screenshot 2024-03-08 at 15 14 54

TOGGLE PÅ:
Screenshot 2024-03-08 at 15 14 39

@idaame
Copy link
Contributor

idaame commented Mar 11, 2024

Dette går ikke på det dere har lagt inn, men mer på foreslått layout: burde det være noe lenke i den infoboksen om utvidet barnetrygd til et sted man kan lese mer om hva det er? Feks at utvidet barnetrygd er en lenke til info-sidene på nav

Comment on lines +45 to +55
const Layout = styled.div`
max-width: var(--innhold-bredde);
display: grid;
gap: 3rem;
margin: 2rem auto 4rem auto;

@media all and ${device.tablet} {
max-width: 100%;
margin: 2rem 2rem 4rem 2rem;
}
`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Kunne man heller ha brukt VStack her?

Copy link
Contributor

Choose a reason for hiding this comment

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

Trenger man egentlig å sjekke om device er tablet her?

Copy link
Contributor

Choose a reason for hiding this comment

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

Vi snakket om det! Men måtte uansett ha lagt på resten av stylingen, så gikk bort fra det 🤔 om du foretrekker at vi bruker komponenten allikevel kan vi gjøre det

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, det med tablet er gammelt og bare videreført

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah skjønner! Men trenger vi egentlig å bruke grid her? Hvis jeg har skjønt det riktig så skal alt innholdet alltid være under hverandre, og da er vel det egentlig ikke nødvendig 🤔 Personlig tror jeg at jeg foretrekker å bruke VStack, men her er det ingen fasit, så dere kjører på med det dere syns er best 🙌

Copy link
Contributor Author

@daphneleebeek daphneleebeek Mar 11, 2024

Choose a reason for hiding this comment

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

Vi trenger å ha med bredden, så da må det i såfall bli en styled VStack eller to komponenter 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tror ikke jeg skjønte helt hvorfor vi ikke skulle ha device.tablet 🤔 noen som kan forklare?

Copy link
Contributor

Choose a reason for hiding this comment

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

Jaa, jeg tenkte stylet VStack isåfall, og kanskje ha kalt den StyledVStack i stedet for Layout. Personlig liker jeg disse komponentene fra Aksel fordi det gjør koden lettere å lese da man slipper å bla seg opp til css-koden for å skjønne hva slags komponent det er. Da kunne man droppet display: grid og flyttet gap ned som prop til komponenten. Meeen gir også mening sånn dere har gjort det så med på det også hvis dere foretrekker det 😊

Copy link
Contributor

Choose a reason for hiding this comment

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

Når det gjelder device.tablet var jeg bare litt usikker på hvorfor vi må ha med denne. Ville tenkt at det man ønsker egentlig er å sjekke om skjermen er smalere enn et gitt brekkpunkt. Typ om du har nettleseren veldig smal på PC ønsker vi kanskje også å ha 100% bredde? Men skjønte på svaret til Kristine at det gjøres sånn her ellers i koden, så går fint for meg å bare kjøre på med denne løsningen her også 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah skjønner! Ja, device.tablet er en max-bredde, så er jo egentlig et brekkpunkt 😊 Det går jo an å kun legge inn det hardkodede tallet der og. Synes dog det er litt nice med navn så man har et referansepunkt på hva den størrelsen egentlig betyr.

@daphneleebeek daphneleebeek merged commit 581815d into main Mar 12, 2024
7 checks passed
@daphneleebeek daphneleebeek deleted the feat/endre-info-forside branch March 12, 2024 07:56
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.

4 participants