Conversation
acao commented Jan 11, 2023
acao commented - moved much of the logic to a file that only loads on the appropriate page
- reorganize a bit to make more sense
- added dynamic clientside validation for minimum amount of candidates
acao force-pushed the reorganize-add-validation branch from 095dd40 to d4821bd Compare 
✅ Deploy Preview for techworkersberlin ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for techworkersberlin ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
acao force-pushed the reorganize-add-validation branch 2 times, most recently from 15ca90a to b039f0c Compare 
acao force-pushed the reorganize-add-validation branch from b039f0c to e48a9ec Compare 
| Here's an example - and it updates the validation in realtime if the number of employees field changes |
acao force-pushed the reorganize-add-validation branch from 3f09633 to ea1259c Compare 
shushugah left a comment
shushugah left a comment There was a problem hiding this comment.
Some comments, will look over the javascript refactoring later
| @@ -0,0 +1,14 @@ | |||
| {% if page.namespace != 'index' %} | |||
| <div id="breadcrumbs"> | |||
| {% assign crumbs = page.url | remove: '/index.html' | split: '/' %} | |||
There was a problem hiding this comment.
can remove remove: '/index.html' | since the url never includes that in our cases?
| / {{ crumb | capitalize }} | ||
| {% else %} | ||
| / | ||
| <a href="{% assign crumb_limit = forloop.index | plus: 1 %}{% for crumb in crumbs limit: crumb_limit %}{{ crumb | append: '/' }}{% endfor %}">{{ crumb | replace: '-', ' ' | remove: '.html' | capitalize }}</a> |
There was a problem hiding this comment.
can remove | remove: '.html' here as well
| <meta http-equiv="X-UA-Compatible" content="IE=edge"> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1"> | ||
| <title>{% if page.title %}{{ page.title | escape }}{% else %}{{ site.title | escape }}{% endif %}</title> | ||
| <title>{% if page.title %}{{ page.title | escape }} | {{site.title}} {% else %}{{ site.title | escape }}{% endif %}</title> |
There was a problem hiding this comment.
Why not replace this with page.title | | site.title basically? (need to look up correct syntaxt)
There was a problem hiding this comment.
The idea is to improve SEO by having both the page and site title together. the pipe character appears in the title
| } | ||
| | ||
| input:required::after { | ||
| content: '*' |
There was a problem hiding this comment.
I couldn't get it working yet
| </tr> | ||
| <tbody id="signatures_id"></tbody> | ||
| </table> | ||
| <script type="text/javascript">{% include list-nomination.js %}</script> |
There was a problem hiding this comment.
Brilliant; no longer pollutes the other unrelated html pages.
| type: "events" | ||
| values: | ||
| layout: "event" | ||
| namespace: event |
There was a problem hiding this comment.
Just so I don't miss anything, these namespaces aren't used yet right? or are they for generating the breadcrumb? Because even a null namespace would confirm this is not an index page, the only time you use namespace logic in this PR? For other cases like translation, this would still be useful so can keep
| key: events.label | ||
| - url: /works-councils | ||
| text: Works Councils | ||
| key: works_councils.label |
There was a problem hiding this comment.
This broke the text of menu, needs to be removed or have code updated first
There was a problem hiding this comment.
I did not mean to push changes to this file, my bad
2 participants
