-
Notifications
You must be signed in to change notification settings - Fork 6
Feature/carlos turmo application test html #7
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
base: main
Are you sure you want to change the base?
Feature/carlos turmo application test html #7
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.
Other comments (14)
-
src/scss/main.scss (86-86)
There's a typo in the CSS variable reference. You have `var(----fontColorLight)` with four dashes instead of two.
background-color: var(--fontColorLight); -
src/scss/main.scss (175-175)
The same typo with four dashes appears here as well.
background-color: var(--fontColorLight); - src/scss/main.scss (1-1) The code references font family variables like `var(--fontFamilyFutura)` and `var(--fontFamilyGeorgia)`, but these aren't defined in the :root section. Make sure these variables are properly defined in the imported 'fonts' file.
-
css/main.css (1-1)
There's an incorrect CSS variable reference in the body and footer styles. The variable `--fontColorLight` is being referenced as `var(----fontColorLight)` with extra dashes, which will prevent it from working properly.
body{font-family:var(--fontFamilyFutura);background-color:var(--fontColorLight);color:var(--fontColor);line-height:1.5} -
css/main.css (1-1)
The same CSS variable reference error appears in the footer styles. The variable `--fontColorLight` is being referenced as `var(----fontColorLight)` with extra dashes.
footer .footer-mainfooter{display:flex;justify-content:center;align-items:center;min-height:var(--footerMainHeight);background-color:var(--fontColorLight);padding:var(--generalPadding) 0;text-align:left} -
src/fonts/futura/stylesheet.css (10-17)
There are two `@font-face` declarations with `font-weight: bold` but different source files (lines 1-8 and 10-17). This can cause inconsistent font rendering as browsers might choose either the regular Bold or CondensedExtraBold version when `font-weight: bold` is specified.
Consider using different font-weight values (like
700for Bold and800for CondensedExtraBold) or different font-family names (like 'Futura' and 'Futura Condensed') to ensure consistent rendering across browsers. - src/scss/_fonts.scss (5-5) The font paths are using absolute URLs (starting with '/src/') which might cause issues in production builds where the directory structure could be different. Consider using relative paths (like '../fonts/...') or configuring path aliases in your build system to ensure fonts load correctly across different environments.
- src/scss/_fonts.scss (1-6) The `font-display: swap` property is defined for Futura fonts but missing for Georgia fonts. For consistent font loading behavior across the site, consider adding this property to all font declarations.
- index.html (141-141) The copyright year is hardcoded as 2025, which will become outdated. Consider using JavaScript to dynamically update the year, or at least use a range like "2023-2025".
- src/fonts/georgia/example.html (12-15) Consider moving the inline styles to the external stylesheet (`style.css`) that's already being referenced. This would improve maintainability and follow the separation of concerns principle.
- src/fonts/georgia/example.html (11-11) Consider replacing the hardcoded URL reference with a comment or attribution that doesn't depend on the external site remaining available.
-
src/fonts/georgia/style.css (28-30)
For the 'Georgia Bold Italic' font-face declaration, both the font-style and font-weight properties should be updated to correctly represent the font characteristics.
font-family: 'Georgia Bold Italic'; font-style: italic; font-weight: 700; -
index.html (33-33)
The alt attribute is being misused on anchor tags. The alt attribute is only valid for img elements, not for anchor tags. For links, use title for tooltips (which you already have) and aria-label for accessibility if needed.
<a class="logo" href="/" title="ooptimo"> - src/fonts/georgia/example.html (19-19) Add a newline at the end of the file to follow the common text file convention.
💡 To request another review, post a new comment with "/windsurf-review".
| font-family: 'Georgia Bold'; | ||
| font-style: normal; | ||
| font-weight: normal; |
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.
For the 'Georgia Bold' and 'Georgia Bold Italic' font-face declarations, the font-weight should be set to 'bold' or '700' instead of 'normal' to correctly represent the font weight.
| font-family: 'Georgia Bold'; | |
| font-style: normal; | |
| font-weight: normal; | |
| font-family: 'Georgia Bold'; | |
| font-style: normal; | |
| font-weight: 700; |
| <meta property="og:image" content="http://localhost:3000/src/img/ooptimo-image.jpg" /> | ||
| <meta property="og:image:width" content="1200" /> | ||
| <meta property="og:image:height" content="630" /> | ||
| <meta property="og:locale" content="en" /> |
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's a mismatch between the html lang attribute (ca) and the Open Graph locale (en). These should be consistent for proper localization.
| <meta property="og:locale" content="en" /> | |
| <meta property="og:locale" content="ca" /> |
| <div class="col col-12"> | ||
| <h1>Creiem en una <span>era digital</span>, on el consum tradicional i l’online es fusionen</h1> | ||
| <h2>Connectant les companyies amb els seus clients com mai abans ho havien fet.</h2> | ||
| <button class="btn btn-primary">Contactar</button> |
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 button element should have a type attribute specified. Without it, the default is 'submit' which could cause unexpected behavior if placed inside a form.
| <button class="btn btn-primary">Contactar</button> | |
| <button type="button" class="btn btn-primary">Contactar</button> |
Frontend HTML application test - From Figma to HTML.
A l'hora d'executar el projecte, m'he trovat amb diferents errors relacioats amb versions antigues o obsoletes. Per aquest motiu he decidit muntar el projecte amb Docker ajustat a les característiques del projecte per evitar conflictes.
També he decidit utilitzar l'stack actual, utilitzant SCSS sense afegir cap llibreria adicional per agilitzar el proces evitant configuracions extra.
server.js: Modificat per poder accedir al servidor web des de fora de Docker.
Per executar el projecte amb Docker:
docker build -t ooptimo-test .
docker run -p 3000:3000 ooptimo-test