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

Andrea/Login page #483

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Andrea/Login page #483

wants to merge 12 commits into from

Conversation

andrea-zecevic
Copy link
Contributor

Copy link
Member

@lovretomic lovretomic left a comment

Choose a reason for hiding this comment

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

Evo ostavia san dosta stvari, najvise oko layouta. Baci oko na sve pa rerequestaj review kad fixas. Takoder, prije nego sta rerequestas si mergeaj main u branch.

line-height: 28px;
letter-spacing: -0.24px;
margin-bottom: 24px;
width: 295px;
Copy link
Member

Choose a reason for hiding this comment

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

image
Zbog ovog title izlazi iz boxa pa nije centriran. Opcenito izbjegavat uvk stavljat fixan width na tekstualne elemente.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

display: flex;
flex-direction: row;
justify-content: space-between;
padding: 24px;
Copy link
Member

Choose a reason for hiding this comment

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

image
image
Razmak izmedu naslova i forme je prevelik. Treba sve zajedno bit 48 px, a sam input ima toliku gornju marginu (uz neke paddinge s naslova bude previse). Pogledat zasto title ima donju marginu od 24 px cini mi se da je tu dio problema. I title container ima isto suvisne margine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 18 to 24
if (!email) {
setEmailError('Hej, trebaš ispuniti sva polja.');
isValid = false;
} else {
setEmailError('');
}

Copy link
Member

Choose a reason for hiding this comment

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

Doda bi i validaciju formata maila, tj. da ima i @ i tocke itd. Koristi regex: https://stackoverflow.com/questions/46155/how-can-i-validate-an-email-address-in-javascript

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

justify-content: space-between;
padding: 0px 24px;
gap: 12px;
margin-top: 330px;
Copy link
Member

Choose a reason for hiding this comment

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

image

ideja je da kad si na mobitelu inputi budu gore, a botuni budu priljepljeni za dno ekrana, tj da nema scrolla ukoliko je moguce. Staticki razmak koji je definiran u figmi ustavri ovisi o visini ekrana. Tu bi recimo solucija bila da kad je na mobitelu neka visina pagea bude tocno 100vh pa u flexbox stavit justify content space between izmedu containera za input i ocntainera za botune.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 7 to 20
.blackDiv {
color: $primary-background;
height: 116px;
display: flex;
justify-content: left;
align-items: center;
padding-left: 24px;
padding-top: 60px;

@media (min-width: $breakpoint-tablet) {
justify-content: center;
padding-bottom: 40px;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Ovako, ova crna pozadina bi trebala biti primijenjena na cijeli page jer kad pogledas desktop takoder postoje margine ogo glavnog bijelog boxa po cijeloj stranici. arhitektura bi trebala biti

wrapper
page name
container

i onda za container recimo
margin 24px da se udalji od rubova

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

msm da je done

Comment on lines +76 to +78
<Button variant='orange' onClick={handleLogin}>
Prijavi se
</Button>
Copy link
Member

Choose a reason for hiding this comment

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

Jesi probala istrazit kako napravit da se botun digne iznad tipkovnice na mobitelu?
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ovo san pokusala, ali nisan uspila moran jos istrazit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nasla od ovog lika i prilagodila malo za uredjaje koji ne podrzavaju visualViewport https://github.com/straxico/use-detect-keyboard-open/blob/main/src/index.js

Comment on lines 95 to 98
margin: 0 auto;
min-width: 500px;
padding-top: 48px;
gap: 48px;
Copy link
Member

Choose a reason for hiding this comment

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

image

cijeli content na desktopu treba bit sirok 422px. znaci ustvari width se fixa na 422 i maknu se sve margine/paddingsi

image

ovo je dosta bitno jer se korisnik switchat medu pagevima pa se lako primijetit razlike u sirini kojih bi se tribali drzat na svakom pageu konstantno

Copy link
Member

Choose a reason for hiding this comment

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

Preimenuj u nesto smisleno i stavi malo pocetno slovo. recimo close ili nesto slicno

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

Btw tek san sad primjetia, ikonica oka na inputu se wiggla kad se switcha iz skriveno i neskriveno. samo to isto prilagodi da ostane na mistu kad se toggla

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.

2 participants