-
Notifications
You must be signed in to change notification settings - Fork 0
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
Login page #214
Login page #214
Conversation
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.
Good progress overall 🚀, besides responsiveness and the color issue in tailwind which you are working on, the scaling looks weird on a display with lower vertical resolution (which is the first image in this case, and the case with my laptop 😄 ) maybe the login should scale down accordingly to make the experience more consistent
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #214 +/- ##
==========================================
- Coverage 1.01% 0.86% -0.15%
==========================================
Files 49 51 +2
Lines 1584 1857 +273
Branches 51 53 +2
==========================================
Hits 16 16
- Misses 1521 1792 +271
- Partials 47 49 +2 ☔ View full report in Codecov by Sentry. |
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.
Good job making this work on all desktops 🥳 but I have some concerns about the colors that we talked about in the retrospective
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.
Also, it'd be nice to have this in storybook too |
I tried replicating this placing small icons on top of the input but I couldn't. Could you maybe inspect the page and share the code, or let me know the extension name? |
bfc24da
to
5e7548f
Compare
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.
@BrunoRosendo @LuisDuarte1 pls re-review :) |
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.
really minor stuff
The page looks good IMO and the PR is pretty much ready for merge
src/routes/(app)/login/+page.svelte
Outdated
<br /> | ||
<button><a href="/profile">Profile</a></button> | ||
</form> | ||
<!-- Colocar a página toda scrollable incluindo footer, ver container queries --> |
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.
<!-- Colocar a página toda scrollable incluindo footer, ver container queries --> |
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.
is this still a problem? I don't think so
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.
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.
Ah I see! Open an issue, we can think about a fix later on
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.
I think this is fixed by #278, I thought this was a problem just with the mobile sidebar and didn't notice this case but it's fixed too
@LuisDuarte1 pls re-review |
Closes #140
Initial implementation of member login page
Screenshots
Review checklist