-
Notifications
You must be signed in to change notification settings - Fork 1
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
Tca 23/event details #16
base: develop
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
src/components/elements/Icon.astro
Outdated
width: string; | ||
height: string; | ||
bgColor: string; | ||
rounded: string; |
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.
Can we change the rounded
property to an enum
to catch more errors effectively?
src/components/elements/Icon.astro
Outdated
height={height} | ||
viewBox="0 0 24 24" | ||
fill="none"> | ||
{svgPaths.map((path) => ( |
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.
@CodeLeom, help me understand why we have to loop through an SVG path here.
Are we doing something special with the SVGs?
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.
This is one hell of a long file. Can we break it down into bits and small components?
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.
Most of the font sizes here are not following the design so also spacings
src/pages/events/details.astro
Outdated
</div> | ||
|
||
<!-- event details content for location, attendance, dates, and cost --> | ||
<div class="w-[352px] h-[576px] sm:order-2 order-1"> |
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.
This can be a component on its own
src/pages/events/details.astro
Outdated
</div> | ||
</section> | ||
|
||
<!-- Host & Speakers --> |
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.
This can also be a component
src/pages/events/details.astro
Outdated
</svg> | ||
</a> | ||
<a href="#"> | ||
<svg |
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.
SVGs still happening here again
src/pages/events/details.astro
Outdated
<!-- Who can attend? --> | ||
<section class="flex justify-between items-center sm:w-[1142px] w-[375px] mx-4 mt-16"> | ||
<div class="w-[704px] sm:ml-[149px]"> | ||
<p class="text-xl font-medium mb-4 text-gray-500">Who can attend?</p> |
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.
Again this can be a component of it's own
@CodeLeom is this the PR ready for review? |
@CodeLeom did you pull changes from develop into this your branch? I can see Next.js changes here? |
Yes, I did.
…On Tue, 7 May 2024 at 14:29, Jamin ***@***.***> wrote:
@CodeLeom <https://github.com/CodeLeom> did you pull changes from develop
into this your branch?
I can see Next.js changes here?
—
Reply to this email directly, view it on GitHub
<#16 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIXQRGEPPHPGCA6XFBQN6OLZBDJMBAVCNFSM6AAAAABDDRELMSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJYGQYTGMRRGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
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 are a couple of fixes needed but I'll fix them when I need this page
<!-- Date & Time --> | ||
<div class="flex mt-8"> | ||
<!-- using the icon component --> | ||
<Fragment set:html={CalenderSVG} /> |
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.
Why are we still using this?🤔
We should import icons normally without Fragment
--- | ||
import Button from "../../components/elements/Button.astro"; | ||
import LocationSVG from "../../../public/location.svg?raw"; | ||
import AttendeeSVG from "../../../public/attendee.svg?raw"; |
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.
any reason why we are importing svgs with ?raw
query?
import regIcon from "../../../public/regIcon.svg?raw"; | ||
--- | ||
|
||
<section |
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.
All components should follow this pattern:
<section class="py-20">
<div class="container mx-auto max-w-screen-xl p-5">
...
</div>
<section>
<div class="mr-16"> | ||
<div class="flex items-center"> | ||
<span class="mr-2 inline-flex items-center"> | ||
<Fragment set:html={phone} /> |
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.
Same icon fragment
--- | ||
|
||
<section class="flex justify-between items-center sm:w-[1142px] w-[375px] mx-4 my-16"> | ||
<div class="w-[704px] sm:ml-[149px]"> |
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 see so many custom pixel values here, we should considering using percentage instead for more responsiveness
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.
We don't need this file here😁
Event details page with hero section, about event section, host and speaker details, demography, tags, and contact. A new component was also created for Icon