Feature/contacts #139

jlcrodrigues · 2023-04-05T12:45:01Z

Closes #17

Created the contacts page for both PC and mobile.

Screenshots

Review checklist

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

pedrosilva17

Great work! I have some suggestions that might improve the code a little bit, let me know if you agree with them.

pedrosilva17 · 2023-04-12T11:29:27Z

src/routes/contacts/+page.svelte

+    {#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}


The inline style tags are not necessary and can be applied with tailwind (object-none, object-center, min-wand min-h), although with slightly different px values (for example, h-52 is equal to height:208px; which I would say is close enough). If you really need exactly 200px, you can use h-[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 the min-w and min-hclasses 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 tailwind md:breakpoint, which has the exact same px value as the if condition.

pedrosilva17 · 2023-04-12T11:37:08Z

src/routes/contacts/+page.svelte

+<style>
+    form > input {
+        width: 100%;
+    }
+
+    @media (min-width: 1050px) {
+        #location {
+            width: 1039px;
+        }
+    }
+</style>


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.

pedrosilva17 · 2023-04-12T11:42:46Z

src/routes/contacts/+page.svelte

+            <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>


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, typeand placeholder labels.

The font is actually named source_codeand not source-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.

srissirs

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?

jlcrodrigues · 2023-04-12T17:52:36Z

@srissirs @pedrosilva17 Thank you for the feedback. I think I fixed the problems mentioned. Please take another look.

BrunoRosendo

Good job so far! I have a few suggestions

src/routes/contacts/Graph.svelte

src/routes/contacts/+page.svelte

src/routes/contacts/Graph.svelte

src/routes/contacts/+page.svelte

BrunoRosendo

just small stuff

src/routes/contacts/Graph.svelte

BrunoRosendo · 2023-04-27T07:45:03Z

src/routes/contacts/+page.svelte

+    <source media="(max-width: 768px)" srcset="/images/feup_buildings.svg" />
+    <source media="(min-width: 769px)" srcset="/images/feup_buildings_md.svg" />


why doesn't it use the same pixels?

codecov · 2023-09-27T14:57:21Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (8497122) to head (9e3d0cc).

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.
📢 Have feedback on the report? Share it here.

LuisDuarte1

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 🎉

LuisDuarte1 · 2023-10-08T13:42:11Z

static/images/outline_white.png

Any reason why you've scaled the hexagon outline?

BrunoRosendo

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)

…nd into feature/contacts

MRita443

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

MRita443 · 2024-08-03T11:21:26Z

package-lock.json

-      "version": "6.2.0",
-      "resolved": "https://registry.npmjs.org/tabbable/-/tabbable-6.2.0.tgz",
-      "integrity": "sha512-Cat63mxsVJlzYvN51JmVXIgNoUokrIaT2zLclCXjRd8boZ0004U4KCs/sToJ75C6sdlByWxpYnb5Boif1VSFew=="
-    },


Were these removed dependencies not being used?

I didn't change any of the dependencies, the file was generated like this.

src/lib/components/icons/LabelInput.svelte

src/routes/(app)/contacts/Graph.svelte

MRita443

Seems ready but just some small comments!

MRita443 · 2024-08-23T15:14:46Z

src/lib/components/icons/label-input.svelte

+<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}


I think it'd be beneficial to put this component on storybook

MRita443 · 2024-08-23T15:17:34Z

src/routes/(app)/contacts/graph.svelte

+<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>


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

MRita443 · 2024-08-30T15:52:38Z

@rubuy-74 Storybook tests are failing due to some accessibility checks, pls fix them! To see what they are run storybook and check the accessibility tab at the bottom of the page

MRita443

Everything from my previous review looks great, two small things. The inputs appear as black to me, probably because of my browsers theme.

Second thing, the LabelInput component, when a text area, still gives the option for a placeholder but no placeholder is visible (try it out on storybook)

MRita443 · 2024-09-12T18:46:31Z

@srissirs Please re-review!

…nd into feature/contacts

MRita443

Great job! Having to add the fixed back into the navbar creates the problem of making page content scroll behind it I opened an issue for that, but this page looks great!

Not longer reviewer, problems fixed

marineve17 and others added 11 commits January 22, 2023 19:18

feat: background and navbar

add6519

Added first footer instance

94b55e5

initial sidebar styling

e37b278

improve sidebar logic

0990a65

layout adjustments to better fit the intended design

b11602f

updated footers social networks links

c582672

Added contact form

3158ee8

Added social media graph

ddb7f95

Made adjustments to icon and graph size

35cb072

Merge branch 'develop'

8019294

Added buildings oultine

e11dae9

BrunoRosendo requested a review from a team April 5, 2023 15:30

pedrosilva17 reviewed Apr 12, 2023

View reviewed changes

srissirs previously requested changes Apr 12, 2023

View reviewed changes

jlcrodrigues added 2 commits April 12, 2023 18:28

Made small improvements to contacts page

6e9ac5b

Fixed linter issues

0059096

BrunoRosendo requested changes Apr 16, 2023

View reviewed changes

BrunoRosendo reviewed Apr 16, 2023

View reviewed changes

src/routes/contacts/+page.svelte Outdated Show resolved Hide resolved

Small changes to contacts

a9bab3f

jlcrodrigues requested review from BrunoRosendo, pedrosilva17 and srissirs April 26, 2023 11:07

BrunoRosendo requested changes Apr 27, 2023

View reviewed changes

Merge branch 'develop'

592635b

Fixed errors according to suggestions

83600a2

MRita443 requested review from BrunoRosendo, LuisDuarte1 and MRita443 October 4, 2023 15:14

LuisDuarte1 reviewed Oct 8, 2023

View reviewed changes

static/images/outline_white.png Outdated

Copy link

Member

LuisDuarte1 Oct 8, 2023

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?

BrunoRosendo approved these changes Oct 9, 2023

View reviewed changes

MRita443 added 4 commits February 29, 2024 16:47

Linting and branch update

0b1faa6

Merge branch 'develop' of github.com:NIAEFEUP/website-niaefeup-fronte…

090fec3

…nd into feature/contacts

Fixes to mobile margin overflows

4575213

Added page storybook component

9e3d0cc

MRita443 requested a review from rubuy-74 July 20, 2024 11:32

Merge branch 'develop' into feature/contacts

43a22de

MRita443 requested changes Aug 3, 2024

View reviewed changes

Comply with requested changes

87d5322

MRita443 requested changes Aug 23, 2024

View reviewed changes

Create Storybook Components

0990186

Acessability Change

dbf3dfa

rubuy-74 requested a review from MRita443 August 31, 2024 09:12

MRita443 requested review from srissirs and removed request for srissirs September 12, 2024 18:32

MRita443 requested changes Sep 12, 2024

View reviewed changes

rubuy-74 added 3 commits October 14, 2024 15:45

Minor Changes

0bae58a

Merge branch 'develop' of github.com:NIAEFEUP/website-niaefeup-fronte…

c14c0c7

…nd into feature/contacts

Linter issue

fb97e6b

rubuy-74 requested a review from MRita443 October 16, 2024 08:57

fix navbar hidden buttons

bdb0389

MRita443 approved these changes Nov 5, 2024

View reviewed changes

MRita443 merged commit 4c671ba into develop Nov 5, 2024
5 checks passed

MRita443 deleted the feature/contacts branch November 5, 2024 12:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/contacts #139

Feature/contacts #139

jlcrodrigues commented Apr 5, 2023

pedrosilva17 left a comment

pedrosilva17 Apr 12, 2023 •

edited

Loading

pedrosilva17 Apr 12, 2023

pedrosilva17 Apr 12, 2023

srissirs left a comment

jlcrodrigues commented Apr 12, 2023

BrunoRosendo left a comment

BrunoRosendo left a comment

BrunoRosendo Apr 27, 2023

codecov bot commented Sep 27, 2023 •

edited

Loading

LuisDuarte1 left a comment

LuisDuarte1 Oct 8, 2023

BrunoRosendo left a comment

MRita443 left a comment

MRita443 Aug 3, 2024

rubuy-74 Aug 8, 2024

MRita443 left a comment

MRita443 Aug 23, 2024

MRita443 Aug 23, 2024

MRita443 commented Aug 30, 2024

MRita443 left a comment

MRita443 commented Sep 12, 2024

MRita443 left a comment

		<source media="(max-width: 768px)" srcset="/images/feup_buildings.svg" />
		<source media="(min-width: 769px)" srcset="/images/feup_buildings_md.svg" />

Feature/contacts #139

Feature/contacts #139

Conversation

jlcrodrigues commented Apr 5, 2023

Screenshots

Review checklist

pedrosilva17 left a comment

Choose a reason for hiding this comment

pedrosilva17 Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srissirs left a comment

Choose a reason for hiding this comment

jlcrodrigues commented Apr 12, 2023

BrunoRosendo left a comment

Choose a reason for hiding this comment

BrunoRosendo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 27, 2023 • edited Loading

Codecov Report

LuisDuarte1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrunoRosendo left a comment

Choose a reason for hiding this comment

MRita443 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MRita443 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MRita443 commented Aug 30, 2024

MRita443 left a comment

Choose a reason for hiding this comment

MRita443 commented Sep 12, 2024

MRita443 left a comment

Choose a reason for hiding this comment

pedrosilva17 Apr 12, 2023 •

edited

Loading

codecov bot commented Sep 27, 2023 •

edited

Loading