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

feat: profile component #282

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

feat: profile component #282

wants to merge 7 commits into from

Conversation

dcbwe
Copy link

@dcbwe dcbwe commented Feb 5, 2025

profile.blade.php skapad i resources/views/components/

Copy link
Contributor

@ell-ska ell-ska left a comment

Choose a reason for hiding this comment

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

some changes are needed! would be nice to see a screenshot or video of how this looks also :)

resources/views/profile.blade.php Outdated Show resolved Hide resolved
resources/views/components/profile.blade.php Outdated Show resolved Hide resolved
resources/views/components/profile.blade.php Outdated Show resolved Hide resolved
@ell-ska ell-ska changed the title Profile component feat: profile component Feb 6, 2025
Copy link
Contributor

@ell-ska ell-ska left a comment

Choose a reason for hiding this comment

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

again, would love to see a preview of what this looks like and especially how it behaves responsively

@@ -0,0 +1,54 @@
@props([
'user',
'buttons',
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think you can pass html as a prop? and that this rather should be a named slot. but correct me if i'm wrong!

Copy link
Author

Choose a reason for hiding this comment

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

absolut inte säker?

Copy link
Contributor

Choose a reason for hiding this comment

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

om du har testat och det funkar så är det lugnt men vill veta att det funkar innan vi mergar detta

resources/views/components/profile.blade.php Outdated Show resolved Hide resolved
resources/views/components/profile.blade.php Outdated Show resolved Hide resolved
Comment on lines 18 to 24
<div class="flex size-30 flex-none items-center justify-center">
@isset($user)
<x-avatar :image="$user->image" size="lg" />
@else
<x-avatar size="lg" />
@endisset
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think there needs to be a wrapping div for the avatar. there's also no need to check if there is a $user because this will never be used where there potentially isn't a user. x-avatar handles if the user doesn't have a profile picture

Suggested change
<div class="flex size-30 flex-none items-center justify-center">
@isset($user)
<x-avatar :image="$user->image" size="lg" />
@else
<x-avatar size="lg" />
@endisset
</div>
<x-avatar :image="$user->image" size="lg" />

Copy link
Author

Choose a reason for hiding this comment

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

Är wrapper onödig tar vi bort den. Check finns för att inte krasha i test

Copy link
Contributor

Choose a reason for hiding this comment

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

vilka test? denna komponent används bara på profilsidan och man kan inte gå in på en profil som inte existerar. då kommer man komma till 404 innan denna sida renderar

Copy link
Author

Choose a reason for hiding this comment

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

fixad

@endisset
</div>

<div class="flex grow flex-col gap-3 pr-10 text-slate-50">
Copy link
Contributor

Choose a reason for hiding this comment

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

why is pr-10 needed?

Copy link
Author

Choose a reason for hiding this comment

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

följer figmas design, kan tas bort

Copy link
Contributor

Choose a reason for hiding this comment

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

finns ingen padding på denna div i figma?

Comment on lines 28 to 32
@isset($user)
{{ $user->username }}
@else
@username
@endisset
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@isset($user)
{{ $user->username }}
@else
@username
@endisset
{{ $user->username }}

same reason as above

Copy link
Author

Choose a reason for hiding this comment

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

samma som ovan, kan tas bort

resources/views/components/profile.blade.php Outdated Show resolved Hide resolved
<p class="text-sm font-bold">
{{ $sum }}
</p>
<span class="text-sm font-normal">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<span class="text-sm font-normal">
<span class="text-sm font-normal text-slate-200">

resources/views/components/profile.blade.php Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

again, please don't remove the layout

Copy link
Author

Choose a reason for hiding this comment

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

?

Copy link
Contributor

Choose a reason for hiding this comment

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

du har raderat allt innehåll i denna fil. det ska vara där

@ell-ska
Copy link
Contributor

ell-ska commented Feb 10, 2025

@dcbwe hur går det med denna? Något som är oklart?

@dcbwe
Copy link
Author

dcbwe commented Feb 11, 2025

screenshot_1 (5)
screenshot_1 (4)

@dcbwe dcbwe requested a review from ell-ska February 11, 2025 08:46
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.

create profile component
2 participants