-
Notifications
You must be signed in to change notification settings - Fork 1
1962 recreate samf3 navbar in samf 4 #1966
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
base: master
Are you sure you want to change the base?
Conversation
…mf 3 navbar items. Also split recruitment from the rest of the navbar links / items.
andsamfu
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.
Ser at du må fikse noe på CSS stylingen på mobil, for navbaren går over innholdet:

På mobil om du trykker på "arrangement", så laster den arrangementer siden, men menyen lukker seg ikke av seg selv. Du bytter derfor fra url localhost:3000 til localhost:3000/events, men dette kommer da ikke frem.

Har du husket å ta med dark-/light-mode knappen? Ser den ikke i headeren, her er fra samf4 navbar:
![]()
Her er samf3 gjenskapingen:
![]()
Kan se på koden igjen etter at du fikset opp i disse tingene.
frontend/.env.docker.example
Outdated
|
|
||
| # 'backend' is the name of service container in docker-compose. | ||
| VITE_BACKEND_DOMAIN=http://localhost:8000 | ||
| VITE_CYPRESS_BACKEND_DOMAIN=http://backend:8000 | ||
| VITE_CYPRESS_BASE_URL=http://frontend:3000 | ||
| CYPRESS_BACKEND_DOMAIN=http://backend:8000 |
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.
Forstår ikke hvorfor denne filen ble endret på. Står i kommentaren over at alt må starte med VITE_*
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 usikker selv på ka som har skjedd med filen, lurer på om ide-en min gjør noe rart. Kopierte koden fra master inn i filen slik at den skal være lik. Skal sjekke litt nøye etter ved neste commits om noe rart skjer da 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.
Fiksa tilbakemelding over med mobil display. Har også lagt inn accessability med tabbing, men den er litt funky, burde sees på i et annet issue.
| <> | ||
| <Navbar /> | ||
| <NavbarSamfThree /> |
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.
Vet ikke om det er upopulært, men hva om å kommentere ut samf4 navbar koden istedenfor å slette den? Gjør det enklere å gjenopprette tilbake til samf4 navbar fra samf3 senere.
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.
La til igjen, og la inn todo til å bytte når samf4 navbar skal brukes.
| #navbar_logo_img { | ||
| width: 100%; // QUESTION: Why is this needed? | ||
| padding: 13px; | ||
| } |
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.
Ser ikke behov for denne stylingen, logoen på samfundet.no er 250px wide, og med denne paddingen blir logen 224px istedenfor 250px. Ser ut som om dette er fra samf4 navbaren koden.
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.
Fjerna padding. Stylingen er kopiert og redigert fra samf 4 navbaren ja, siden det tar mye lengre tid å lage en ny fra scratch. Men fortsett å kommenter på kode som ikke brukes, jeg prøver å fjerne det jeg ser, men lett å overse noe :)
| /* Mobile popup navigation */ | ||
| #mobile_popup_container { | ||
| position: fixed; | ||
| top: 60px; | ||
| left: 0; | ||
| width: 100vw; | ||
| height: calc(100vh - 60px); | ||
| min-height: 400px; | ||
| background: $navbar-bg-dark; | ||
| color: white; | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| flex-direction: column; | ||
| z-index: 100; | ||
| @include for-desktop-up { | ||
| display: none; | ||
| } | ||
| } |
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.
Fint om du bruker $navbar-height-samf-three når du har spesifisert det tidligere. Er også forskjellig pixel fra $navbar-height-samf-three på 64px og her på 60px.
| /* Mobile popup navigation */ | |
| #mobile_popup_container { | |
| position: fixed; | |
| top: 60px; | |
| left: 0; | |
| width: 100vw; | |
| height: calc(100vh - 60px); | |
| min-height: 400px; | |
| background: $navbar-bg-dark; | |
| color: white; | |
| display: flex; | |
| align-items: center; | |
| justify-content: center; | |
| flex-direction: column; | |
| z-index: 100; | |
| @include for-desktop-up { | |
| display: none; | |
| } | |
| } | |
| /* Mobile popup navigation */ | |
| #mobile_popup_container { | |
| position: fixed; | |
| top: $navbar-height-samf-three; | |
| left: 0; | |
| width: 100vw; | |
| height: calc(100vh - $navbar-height-samf-three); | |
| min-height: 400px; | |
| background: $navbar-bg-dark; | |
| color: white; | |
| display: flex; | |
| align-items: center; | |
| justify-content: center; | |
| flex-direction: column; | |
| z-index: 100; | |
| @include for-desktop-up { | |
| display: none; | |
| } | |
| } |
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.
Fiksa.
|
|
||
| // Font | ||
| $navbar-font-family: 'futura-pt', 'Lato', sans-serif; | ||
| $navbar-height-samf-three: 64px; |
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.
Ser at samf3 bruker 64px, men nye navbar samf4 er definert som 60px i _constant.scss som $navbar-height. Fant en fil som bruker den gamle høyden SamfOutlet.module.scss, og et sted i denne filen som jeg kommenterte.
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.
Oppdaterte i constants.ts, men det må huskes å reverte når samf 4 navbaren skal brukes. Har lagt inn todo kommentarer på dette.
Not quite the same navbar, but similar enough