Skip to content
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

reorganize, add realtime validation #218

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

acao
Copy link
Contributor

@acao acao commented Jan 11, 2023

  • 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 acao requested a review from shushugah January 11, 2023 23:40
@acao acao force-pushed the reorganize-add-validation branch from 095dd40 to d4821bd Compare January 11, 2023 23:41
@netlify
Copy link

netlify bot commented Jan 11, 2023

Deploy Preview for techworkersberlin ready!

Name Link
🔨 Latest commit 095dd40
🔍 Latest deploy log https://app.netlify.com/sites/techworkersberlin/deploys/63bf49044cee8b00095ddf66
😎 Deploy Preview https://deploy-preview-218--techworkersberlin.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Jan 11, 2023

Deploy Preview for techworkersberlin ready!

Name Link
🔨 Latest commit ea1259c
🔍 Latest deploy log https://app.netlify.com/sites/techworkersberlin/deploys/63c3208368a4590008f8373d
😎 Deploy Preview https://deploy-preview-218--techworkersberlin.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@acao acao force-pushed the reorganize-add-validation branch 2 times, most recently from 15ca90a to b039f0c Compare January 12, 2023 23:29
@acao acao force-pushed the reorganize-add-validation branch from b039f0c to e48a9ec Compare January 12, 2023 23:31
@acao
Copy link
Contributor Author

acao commented Jan 12, 2023

Here's an example - and it updates the validation in realtime if the number of employees field changes
image

@acao acao force-pushed the reorganize-add-validation branch from 3f09633 to ea1259c Compare January 14, 2023 21:37
Copy link
Member

@shushugah shushugah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: '/' %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can remove | remove: '.html' here as well

@@ -2,7 +2,7 @@
<meta charset="utf-8">
<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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not replace this with page.title | | site.title basically? (need to look up correct syntaxt)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: '*'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing this works now!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't get it working yet

@@ -69,3 +82,4 @@ As soon as the Works Council election date is announced, any employee interested
</tr>
<tbody id="signatures_id"></tbody>
</table>
<script type="text/javascript">{% include list-nomination.js %}</script>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brilliant; no longer pollutes the other unrelated html pages.

@@ -92,7 +92,9 @@ defaults:
type: "events"
values:
layout: "event"
namespace: event
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

- url: /works-councils
text: Works Councils
key: works_councils.label
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This broke the text of menu, needs to be removed or have code updated first

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not mean to push changes to this file, my bad

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants