-
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
Feature/contacts #139
base: develop
Are you sure you want to change the base?
Feature/contacts #139
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.
Great work! I have some suggestions that might improve the code a little bit, let me know if you agree with them.
src/routes/contacts/+page.svelte
Outdated
{#if windowWidth >= 768} | ||
<img | ||
alt="Feup Buildings Outline" | ||
src="/images/feup_buildings_md.svg" | ||
class="align-center justify-self-center w-full" | ||
style="min-width:400px; height:200px; object-fit: none; object-position: center;" | ||
|
||
/> | ||
{:else} | ||
<img | ||
alt="Feup Buildings Outline" | ||
src="/images/feup_buildings.svg" | ||
class="align-center justify-self-center w-full" | ||
style="min-width:400px; min-height:160px; object-fit: none; object-position: center;" | ||
|
||
/> | ||
{/if} |
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.
- The inline style tags are not necessary and can be applied with tailwind (
object-none
,object-center
,min-w
andmin-h
), although with slightly different px values (for example,h-52
is equal toheight:208px;
which I would say is close enough). If you really need exactly 200px, you can useh-[200px]
, which is not as clean as the previous tailwind class. Moreover, some classes are not very useful from what I've seen, like the min-widths and min-heights (the latter of which can just be converted to height in this case, I think, as the difference is not noticeable). Again, if you feel like they should be preserved, you can pass an arbitrary value to themin-w
andmin-h
classes using square brackets like I did above. - This if/else statement can be replicated using a
<picture>
tag (read more about it here). Avoids repeating common parts of the image, like the alt text and almost all classes, and doesn't need to use a variable that stores the width of the window, instead changing the source of the image based on a breakpoint. The tag is supported by every modern browser (post-2015), but we can discuss its usage in the meeting later today. For the classes that change depending on the window width, just use the tailwindmd:
breakpoint, which has the exact same px value as theif
condition.
src/routes/contacts/+page.svelte
Outdated
<style> | ||
form > input { | ||
width: 100%; | ||
} | ||
|
||
@media (min-width: 1050px) { | ||
#location { | ||
width: 1039px; | ||
} | ||
} | ||
</style> |
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.
These can be achieved with tailwind as well.
- Use
w-full
in each input's class - Use
lg:w-[1039px]
inside the span's class (the breakpoint targets devices with 1024px of width or greater and sets the width of the span to 1039px in those cases).
Keep in mind the arbitrary value attribution with square brackets is reserved for situations where there are no native similar values, which is the case here.
src/routes/contacts/+page.svelte
Outdated
<label class="m-1 text-white font-source-code" for="email">// Email</label><br> | ||
<input class="rounded-lg p-1 text-primary placeholder-primary" type="email" id="email" placeholder="[email protected]"><br> | ||
<label class="m-1 text-white font-source-code" for="name">// Nome</label><br> | ||
<input class="rounded-lg p-1 text-primary placeholder-primary" type="text" id="name"><br> | ||
<label class="m-1 text-white font-source-code" for="subject">// Assunto</label><br> | ||
<input class="rounded-lg p-1 text-primary placeholder-primary" type="text" id="subject"><br> | ||
<label class="m-1 text-white font-source-code" for="message">// Mensagem</label><br> | ||
<textarea class="rounded-lg p-1 text-primary placeholder-primary" id="message"></textarea><br> |
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.
- Maybe each label/input pair could be converted into a component? I imagine we could potentially have inputs like these in other places of the website, plus it improves readability and reduces code repetition. The parameters to be passed could be the
for
,type
andplaceholder
labels. - The font is actually named
source_code
and notsource-code
, I changed it in my commit to avoid putting brackets around the name in tailwind config file. In hindsight, that might not have been the best decision, as it looks strange in a tailwind class. Maybe it could be reverted and you don't need to change the name here, but just know that the font is not being rendered properly with this code. We'll discuss that in the meeting.
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.
It's looking good! Though I think there should be more space between the fields. The titles are a bit too close to the field directly above. Also, the graph in the mobile version does not seem to be centered. Could you fix that too?
@srissirs @pedrosilva17 Thank you for the feedback. I think I fixed the problems mentioned. Please take another look. |
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 so far! I have a few suggestions
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.
just small stuff
src/routes/contacts/+page.svelte
Outdated
<source media="(max-width: 768px)" srcset="/images/feup_buildings.svg" /> | ||
<source media="(min-width: 769px)" srcset="/images/feup_buildings_md.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.
why doesn't it use the same pixels?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #139 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 16 16
Branches 4 4
=========================================
Hits 16 16 ☔ 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.
Overall it looks great, great job 🚀, there's some issues with z-index and the hexagon outline, which I believe that are already fixed on develop
? So try to merge and see if that's fixed ^^. Just have some minor concern below, but great job otherwise 🎉
static/images/outline_white.png
Outdated
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 you've scaled the hexagon outline?
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'm approving since the feature is looking great! There are just a few side things missing:
- Fix lint
- Create storybook stories and tests (I'll leave this as the team's discretion, since you may want to save time on this)
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.
Looking good overall, a couple file placement and file name consistency changes since the restructuring, and please add the new component to storybook. Otherwise (this might be a bit nit picky) but I noticed when scaling the page at one point I shrink the page and the forms gets bigger which looks a bit odd. Is this easily fixable? If not I say we keep it this way
Screencast.from.03-08-2024.12.44.24.webm
"version": "6.2.0", | ||
"resolved": "https://registry.npmjs.org/tabbable/-/tabbable-6.2.0.tgz", | ||
"integrity": "sha512-Cat63mxsVJlzYvN51JmVXIgNoUokrIaT2zLclCXjRd8boZ0004U4KCs/sToJ75C6sdlByWxpYnb5Boif1VSFew==" | ||
}, |
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.
Were these removed dependencies not being used?
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 didn't change any of the dependencies, the file was generated like this.
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.
Seems ready but just some small comments!
<script> | ||
export let label = ''; | ||
export let id = ''; | ||
export let type = 'text'; | ||
export let placeholder = ''; | ||
export let isTextArea = false; | ||
</script> | ||
|
||
<label class="m-1 font-source_code font-bold text-white" for={id}>{label}</label><br class="mb-1" /> | ||
{#if isTextArea} | ||
<textarea | ||
class="mb-2 w-full rounded-lg p-2 font-source_code text-primary placeholder-primary" | ||
{id} | ||
rows="4" | ||
/><br /> | ||
{:else} | ||
<input | ||
class="mb-2 w-full rounded-lg p-2 text-primary placeholder-primary" | ||
{type} | ||
{id} | ||
{placeholder} | ||
/><br /> | ||
{/if} |
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 it'd be beneficial to put this component on storybook
<script lang="ts"> | ||
import Icon from '@/lib/components/icons/icon.svelte'; | ||
import Icons from '$lib/components/icons/icons'; | ||
import { copyToClipboard } from '$lib/utils'; | ||
|
||
const coords = [ | ||
[1, 1], | ||
[6, 3], | ||
[2, 5], | ||
[10, 6], | ||
[1, 8], | ||
[7, 8] | ||
]; | ||
const lines = [ | ||
[0, 2], | ||
[2, 1], | ||
[1, 5], | ||
[5, 3], | ||
[2, 4] | ||
]; | ||
let innerWidth = 0; | ||
let coefficient = 8; | ||
let iconSize = 8; | ||
|
||
const socials = [ | ||
{ url: 'https://www.instagram.com/niaefeup/', icon: Icons.Instagram, label: 'Instagram' }, | ||
{ url: 'https://github.com/NIAEFEUP', icon: Icons.Github, label: 'Github' }, | ||
{ url: 'https://www.facebook.com/NIAEFEUP', icon: Icons.Facebook, label: 'Facebook' }, | ||
{ url: '[email protected]', icon: Icons.Mail, label: 'Email' }, | ||
{ url: 'https://www.linkedin.com/company/nifeup', icon: Icons.Linkedin, label: 'Linkedin' }, | ||
{ url: 'https://twitter.com/niaefeup', icon: Icons.Twitter, label: 'Twitter' } | ||
]; | ||
</script> | ||
|
||
<svelte:window bind:innerWidth /> | ||
|
||
<svg style="height: 40dvh; min-width: 35dvw" viewBox="0 0 87 80" xmlns="http://www.w3.org/2000/svg"> | ||
<!-- Draw graph edges. --> | ||
{#each lines as line} | ||
<line | ||
x1={coords[line[0]][0] * coefficient} | ||
y1={coords[line[0]][1] * coefficient} | ||
x2={coords[line[1]][0] * coefficient} | ||
y2={coords[line[1]][1] * coefficient} | ||
class="stroke-vivid-red-900" | ||
/> | ||
{/each} | ||
|
||
<!-- Draw graph nodes. --> | ||
{#each coords as coord, index} | ||
<foreignObject | ||
x={coord[0] * coefficient - (iconSize + 2) / 2} | ||
y={coord[1] * coefficient - (iconSize + 2) / 2} | ||
width={iconSize + 2} | ||
height={iconSize + 2} | ||
> | ||
{#if socials[index].url.startsWith('https')} | ||
<div class="flex h-full w-full items-center justify-center rounded bg-white bg-opacity-30"> | ||
<Icon | ||
src={socials[index].icon} | ||
color="white" | ||
size="{iconSize.toString()}px" | ||
href={socials[index].url} | ||
ariaLabel={socials[index].label} | ||
/> | ||
</div> | ||
{:else} | ||
<div | ||
class="flex h-full w-full items-center justify-center rounded bg-white bg-opacity-30" | ||
on:click={() => copyToClipboard(socials[index].url)} | ||
on:keydown={() => copyToClipboard(socials[index].url)} | ||
role="button" | ||
tabindex="0" | ||
aria-label="copy-mail" | ||
> | ||
<Icon src={socials[index].icon} color="white" size="{iconSize.toString()}px" /> | ||
</div> | ||
{/if} | ||
</foreignObject> | ||
{/each} | ||
</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.
Although less reusable it'd be nice to have this on storybook too, for example to test responsivity easily :) And please place this in src/routes/(app)/contacts/_components
for consistency
Closes #17
Created the contacts page for both PC and mobile.
Screenshots
Review checklist