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

Login page #214

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from
Open

Login page #214

wants to merge 20 commits into from

Conversation

MRita443
Copy link
Collaborator

Closes #140

Initial implementation of member login page

Screenshots

image

Review checklist

  • Contains enough appropriate tests
  • Behavior is as expected
  • Clean, well-structured code

Copy link
Member

@LuisDuarte1 LuisDuarte1 left a 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
Copy link

codecov bot commented Jul 14, 2023

Codecov Report

Attention: Patch coverage is 0% with 146 lines in your changes missing coverage. Please review.

Project coverage is 0.86%. Comparing base (2fb1530) to head (0af8cb5).
Report is 11 commits behind head on develop.

Files Patch % Lines
...n/_components/variable-visibility-input.stories.ts 0.00% 47 Missing and 1 partial ⚠️
src/routes/(app)/login/+page.svelte 0.00% 36 Missing ⚠️
...login/_components/variable-visibility-input.svelte 0.00% 23 Missing and 1 partial ⚠️
src/routes/(app)/login/page.stories.ts 0.00% 14 Missing and 1 partial ⚠️
src/lib/components/icons/icon.svelte 0.00% 4 Missing ⚠️
src/lib/components/icons/icons.ts 0.00% 4 Missing ⚠️
src/routes/(app)/_components/layout/sidebar.svelte 0.00% 4 Missing ⚠️
src/routes/(app)/(home)/page.stories.ts 0.00% 2 Missing ⚠️
src/routes/(app)/+layout.svelte 0.00% 2 Missing ⚠️
src/routes/(app)/layout.stories.ts 0.00% 2 Missing ⚠️
... and 4 more
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.
📢 Have feedback on the report? Share it here.

Copy link
Member

@LuisDuarte1 LuisDuarte1 left a 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

tailwind.config.cjs Outdated Show resolved Hide resolved
@BrunoRosendo BrunoRosendo requested a review from a team July 26, 2023 01:28
Copy link
Member

@BrunoRosendo BrunoRosendo left a comment

Choose a reason for hiding this comment

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

I'm not sure why but the small icon my password manager adds really screws up the password container. This is a bit concerning since I've never seen this before and password managers are pretty popular. Can you investigate?

image

@@ -13,5 +13,5 @@
<Icon {src} {color} {size} />
</a>
{:else}
<Icon {src} {color} {size} />
<Icon {src} {size} color="currentcolor" className={$$props.color} />
Copy link
Member

Choose a reason for hiding this comment

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

Why do you use $$props.color? Isn't simply color the same? Maybe I'm missing something here

Copy link
Member

Choose a reason for hiding this comment

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

I'd also appreciate an explanation about className, it just reminds me of react xd

Copy link
Collaborator Author

@MRita443 MRita443 Aug 8, 2023

Choose a reason for hiding this comment

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

I'd also appreciate an explanation about className, it just reminds me of react xd

image

It's just how the icon pack declares it ahah, otherwise I get a class does not exist in type error.
Just tried it myself, I think class is a reserved word

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why do you use $$props.color? Isn't simply color the same? Maybe I'm missing something here

Yup! Will change it. Also thinking of instead of this line simply adding a className property, for when the color trying to be used is not recognized by the color property (eg. custom names, different opacity notation). This way the color wouldn't have to be something like fill-rose-950 and could actually just be a hex or color xd. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

@MRita443 I think className is specific to the Icon library we use. I prefer to use $$props.class and map it to className in Icon.svelte, so we have the same syntax for every component. But I'm willing to hear more opinions

</script>

<div class="grid">
<input {type} {...$$restProps} class="col-start-1 col-end-3 row-start-1 {$$props.class}" />
Copy link
Member

Choose a reason for hiding this comment

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

I didn't know restProps, what it the goal here? I searched a little bit and it doesn't seem to be recommended sveltejs/svelte#8249

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My goal here was to make the component reusable in different scenarios, but maintain the same icon placement. For that I had to have these specific classes, but still wanted to allow for more customization, hence the $$props.class. The $$restProps was to collect all the other attributes like names, placeholders, values etc. I decided to use this because inputs have lots of possible attributes, but I guess I could try to make the most common ones into optional properties maybe?

Copy link
Member

Choose a reason for hiding this comment

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

After searching a bit more about restProps, I believe this implementation is appropriate. Svelte has since updates the docs on this:

$$props references all props that are passed to a component, including ones that are not declared with export. Using $$props will not perform as well as references to a specific prop because changes to any prop will cause Svelte to recheck all usages of $$props. But it can be useful in some cases – for example, when you don't know at compile time what props might be passed to a component.

<Widget {...$$props} />

$$restProps contains only the props which are not declared with export. It can be used to pass down other unknown attributes to an element in a component. It shares the same performance characteristics compared to specific property access as $$props.

<input {...$$restProps} />

This means that specific prop access could be slower (such as $$props.class). I found an alternative but I don't find it visually appealing. Let me know what you think:

export {_class as class};
let _class = "";

<input {type} {...$$restProps} class="{_class}" />

@BrunoRosendo
Copy link
Member

Also, it'd be nice to have this in storybook too

@MRita443
Copy link
Collaborator Author

MRita443 commented Aug 8, 2023

I'm not sure why but the small icon my password manager adds really screws up the password container. This is a bit concerning since I've never seen this before and password managers are pretty popular. Can you investigate?

image

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?

Comment on lines 5 to 13
export default {
title: 'Pages/Login',
component: Page,
parameters: {
layout: 'fullscreen',
backgrounds: { default: 'clear' }
},
decorators: [() => Layout, () => LayoutDecorator]
};
Copy link
Member

Choose a reason for hiding this comment

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

It'd be nice to test the visibility input

Copy link
Member

@LuisDuarte1 LuisDuarte1 left a comment

Choose a reason for hiding this comment

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

Nice progress, it's not 100% responsive yet though, as you can see on my phone, the login is not properly centered and overlapping the hamburger menu:

Also it seems that you have an issue with importing layout from login/page.stories.ts that causes storybook to fail 😅

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.

auth: implement login page
3 participants