team-5/website-styling #33

Open
niyara-tavaresramos wants to merge 38 commits from team-5/website-styling into team-2/develop
No description provided.
# Conflicts:
#	templates/pages/main/index.html.twig
# Conflicts:
#	templates/pages/main/about.html.twig
rileydeman left a comment

Mooi dat de profielpagina zo is opgeruimd. De inline style="..."-attributen zijn vervangen door classes, en de locatie-popup en de projectkaarten zien er nu als één geheel uit. De account-centre-kaarten in een grid met een responsive breakpoint zijn ook een duidelijke verbetering.

Dit moet nog aangepast worden:

Moet opgelost worden

  1. Er zijn merge-conflicten met team-2/develop in public/assets/main/css/about.css, public/assets/main/css/index.css, templates/pages/main/about.html.twig en templates/pages/main/index.html.twig. Team 2 heeft na jouw merge van hun branches nog een nieuwe versie van de about- en homepagina gemerged (PR #34). Merge team-2/develop in je branch en los de conflicten op. Houd daarbij de ux_icon(...)-iconen en path('main.home') uit hun versie aan in plaats van de emoji's en losse links.
  2. .env.local.example wordt in commit dcea15f verwijderd. Andere teams gebruiken dat bestand om hun lokale omgeving op te zetten. Zet het terug.
  3. In dezelfde commit is een leeg bestand git aan de root toegevoegd, waarschijnlijk door een typfout in de terminal. Haal het weg.
  4. templates/pages/main/index.html.twig toont nu een blok "HNNW in cijfers" met 1.200+ leden, 340 projecten en 85% snellere matching. Die cijfers kloppen niet, want het platform zit in open bèta. Op een publieke site is dat misleidend. Haal het blok weg, of vul het later met echte data uit de database.
  5. In index.html.twig laad je core.css opnieuw, ná index.css, terwijl layouts/main.base.html.twig hem al laadt. Omdat hij nu als laatste geladen wordt, overschrijft hij je homepage-styling (bijv. de :root-variabelen en body). Haal die regel weg. In about.html.twig staat about.css twee keer: haal er één weg.
  6. core.css, home.css, index.css en about.css hebben elk een eigen :root, met verschillende waarden voor dezelfde variabele. --background is bijvoorbeeld #010412 in shared/css/style.css en core.css, maar #020617 in home.css en index.css. Ze hebben ook elk een eigen * {}- en body {}-reset. Welke kleur je ziet, hangt dus af van de laadvolgorde. Zet de variabelen op één plek (shared/css/style.css) en haal de dubbele :root/*/body-blokken uit de pagina-CSS.
  7. home.css is eigenlijk de CSS van de profielpagina, met globale regels als * { margin: 0; padding: 0; text-decoration: none; } en body { display: flex; ... }. Je laadt hem nu ook in contact.html.twig, projects.html.twig en account-centre/index.html.twig. Daardoor kan de opmaak van die pagina's (en van de header) ongemerkt veranderen. Hernoem hem naar bijv. profile.css en laad hem alleen op de profielpagina. In account-centre/index.html.twig laad je ook core.css en home.css van de main site, terwijl ac.base.html.twig een eigen stylesheet heeft. Is dat bewust?

Verbeterpunten
8. De homepage-CTA "Maak een account aan" (path('auth.register')) is vervangen door "Nu aan de slag" met mailto:contact@hnnw.nl. Daarmee verdwijnt de enige registratielink op de homepage. Ook zijn de ingevulde seoKeywords en seoDescription leeggemaakt, en de titel is van "Samenwerken aan publieke opgaven" naar "Home" gegaan. Was dat de bedoeling? Zo niet, zet ze terug.
9. account-centre/css/style.css gebruikt var(--dark-bg) en var(--hnnw-secondary-text). --dark-bg staat alleen in de admin- en org-CSS, en --hnnw-secondary-text bestaat nergens. De kaarten krijgen dus niet de achtergrond en tekstkleur die je bedoelt. Definieer ze in shared/css/style.css of gebruik bestaande variabelen.
10. templates/core/account-centre/_navigation.html.twig is in zijn geheel uitgecommentarieerd met {# #}. Wil je de navigatie weg hebben? Haal dan de include uit ac.base.html.twig en verwijder het bestand, in plaats van dode code te laten staan. Overleg dat wel even, want het raakt elke account-centre-pagina.
11. about.html.twig en profile.html.twig hebben een eigen <main>, terwijl main.base.html.twig de body al in een <main> zet. Twee geneste <main>-elementen zijn ongeldige HTML en verwarrend voor screenreaders. Gebruik een <div> of <section>. Op de profielpagina staan ook nog placeholders (This is an example text., src="" bij afbeeldingen, href="#"). Prima voor nu, maar zet er een TODO bij.
12. Deze PR heeft 19 commits, waaronder merges van twee andere team-2-branches, en geen beschrijving. Splits hem liever op: (a) de styling van home en about naar team-2/develop, en (b) core.css, header, profiel en Account Centre als losse PR naar develop. Zet bij beide in een paar zinnen wat je hebt veranderd, met een screenshot.

Let op: deze PR wijzigt gedeelde bestanden die ook in andere PR's veranderen. PR #12 (team 3) wijzigt ook core.css, _header.html.twig en account-centre/index.html.twig. PR #15 (team 6) wijzigt home.css. PR #53 (team-2/develop → develop) bevat de about- en homepagina waar jij op voortbouwt. Stem af wie wat doet, en merge wijzigingen aan core.css en de header pas als de andere teams ervan weten.

Mooi dat de profielpagina zo is opgeruimd. De inline `style="..."`-attributen zijn vervangen door classes, en de locatie-popup en de projectkaarten zien er nu als één geheel uit. De account-centre-kaarten in een grid met een responsive breakpoint zijn ook een duidelijke verbetering. Dit moet nog aangepast worden: **Moet opgelost worden** 1. Er zijn merge-conflicten met `team-2/develop` in `public/assets/main/css/about.css`, `public/assets/main/css/index.css`, `templates/pages/main/about.html.twig` en `templates/pages/main/index.html.twig`. Team 2 heeft na jouw merge van hun branches nog een nieuwe versie van de about- en homepagina gemerged (PR #34). Merge `team-2/develop` in je branch en los de conflicten op. Houd daarbij de `ux_icon(...)`-iconen en `path('main.home')` uit hun versie aan in plaats van de emoji's en losse links. 2. `.env.local.example` wordt in commit `dcea15f` verwijderd. Andere teams gebruiken dat bestand om hun lokale omgeving op te zetten. Zet het terug. 3. In dezelfde commit is een leeg bestand `git` aan de root toegevoegd, waarschijnlijk door een typfout in de terminal. Haal het weg. 4. `templates/pages/main/index.html.twig` toont nu een blok "HNNW in cijfers" met `1.200+ leden`, `340 projecten` en `85% snellere matching`. Die cijfers kloppen niet, want het platform zit in open bèta. Op een publieke site is dat misleidend. Haal het blok weg, of vul het later met echte data uit de database. 5. In `index.html.twig` laad je `core.css` opnieuw, ná `index.css`, terwijl `layouts/main.base.html.twig` hem al laadt. Omdat hij nu als laatste geladen wordt, overschrijft hij je homepage-styling (bijv. de `:root`-variabelen en `body`). Haal die regel weg. In `about.html.twig` staat `about.css` twee keer: haal er één weg. 6. `core.css`, `home.css`, `index.css` en `about.css` hebben elk een eigen `:root`, met verschillende waarden voor dezelfde variabele. `--background` is bijvoorbeeld `#010412` in `shared/css/style.css` en `core.css`, maar `#020617` in `home.css` en `index.css`. Ze hebben ook elk een eigen `* {}`- en `body {}`-reset. Welke kleur je ziet, hangt dus af van de laadvolgorde. Zet de variabelen op één plek (`shared/css/style.css`) en haal de dubbele `:root`/`*`/`body`-blokken uit de pagina-CSS. 7. `home.css` is eigenlijk de CSS van de profielpagina, met globale regels als `* { margin: 0; padding: 0; text-decoration: none; }` en `body { display: flex; ... }`. Je laadt hem nu ook in `contact.html.twig`, `projects.html.twig` en `account-centre/index.html.twig`. Daardoor kan de opmaak van die pagina's (en van de header) ongemerkt veranderen. Hernoem hem naar bijv. `profile.css` en laad hem alleen op de profielpagina. In `account-centre/index.html.twig` laad je ook `core.css` en `home.css` van de main site, terwijl `ac.base.html.twig` een eigen stylesheet heeft. Is dat bewust? **Verbeterpunten** 8. De homepage-CTA "Maak een account aan" (`path('auth.register')`) is vervangen door "Nu aan de slag" met `mailto:contact@hnnw.nl`. Daarmee verdwijnt de enige registratielink op de homepage. Ook zijn de ingevulde `seoKeywords` en `seoDescription` leeggemaakt, en de titel is van "Samenwerken aan publieke opgaven" naar "Home" gegaan. Was dat de bedoeling? Zo niet, zet ze terug. 9. `account-centre/css/style.css` gebruikt `var(--dark-bg)` en `var(--hnnw-secondary-text)`. `--dark-bg` staat alleen in de admin- en org-CSS, en `--hnnw-secondary-text` bestaat nergens. De kaarten krijgen dus niet de achtergrond en tekstkleur die je bedoelt. Definieer ze in `shared/css/style.css` of gebruik bestaande variabelen. 10. `templates/core/account-centre/_navigation.html.twig` is in zijn geheel uitgecommentarieerd met `{# #}`. Wil je de navigatie weg hebben? Haal dan de include uit `ac.base.html.twig` en verwijder het bestand, in plaats van dode code te laten staan. Overleg dat wel even, want het raakt elke account-centre-pagina. 11. `about.html.twig` en `profile.html.twig` hebben een eigen `<main>`, terwijl `main.base.html.twig` de body al in een `<main>` zet. Twee geneste `<main>`-elementen zijn ongeldige HTML en verwarrend voor screenreaders. Gebruik een `<div>` of `<section>`. Op de profielpagina staan ook nog placeholders (`This is an example text.`, `src=""` bij afbeeldingen, `href="#"`). Prima voor nu, maar zet er een TODO bij. 12. Deze PR heeft 19 commits, waaronder merges van twee andere team-2-branches, en geen beschrijving. Splits hem liever op: (a) de styling van home en about naar `team-2/develop`, en (b) `core.css`, header, profiel en Account Centre als losse PR naar `develop`. Zet bij beide in een paar zinnen wat je hebt veranderd, met een screenshot. Let op: deze PR wijzigt gedeelde bestanden die ook in andere PR's veranderen. PR #12 (team 3) wijzigt ook `core.css`, `_header.html.twig` en `account-centre/index.html.twig`. PR #15 (team 6) wijzigt `home.css`. PR #53 (team-2/develop → develop) bevat de about- en homepagina waar jij op voortbouwt. Stem af wie wat doet, en merge wijzigingen aan `core.css` en de header pas als de andere teams ervan weten.
This pull request has changes conflicting with the target branch.
  • public/assets/main/css/about.css
  • public/assets/main/css/index.css
  • templates/pages/main/about.html.twig
  • templates/pages/main/index.html.twig
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin team-5/website-styling:team-5/website-styling
git switch team-5/website-styling

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch team-2/develop
git merge --no-ff team-5/website-styling
git switch team-5/website-styling
git rebase team-2/develop
git switch team-2/develop
git merge --ff-only team-5/website-styling
git switch team-5/website-styling
git rebase team-2/develop
git switch team-2/develop
git merge --no-ff team-5/website-styling
git switch team-2/develop
git merge --squash team-5/website-styling
git switch team-2/develop
git merge --ff-only team-5/website-styling
git switch team-2/develop
git merge team-5/website-styling
git push origin team-2/develop
Sign in to join this conversation.
No reviewers
No labels
No milestone
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
HNNW/core!33
No description provided.