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

[CRAFT-5] cleanup all duplicate fields, optimize all content builder blocks to reuse fields where possible, remove supertable #371

Merged
merged 2 commits into from
Jul 22, 2024

Conversation

Numkil
Copy link
Contributor

@Numkil Numkil commented Jul 18, 2024

Description

This removes all duplicate fields that were auto generated by upgrading craft 4 to craft 5. Optimized all content builder blocks to reuse basic fields wherever possible.

Also removes super-table as it is no longer used in the whole website.

Reason for this change

We had 5 "image" fields that were all duplicates of each other. we had "4" intro fields which were identical. I have now put this all back to 1 each. Also did a project-config rebuild for optimizing the config

@Numkil Numkil changed the title [CRAFT-5] cleanup all duplicate fields, optimize all content builder blocks to reuse fields where possible [CRAFT-5] cleanup all duplicate fields, optimize all content builder blocks to reuse fields where possible, remove supertable Jul 18, 2024
@SarahOorts
Copy link
Contributor

Ik ben er lokaal al eens doorgegaan en had een paar opmerkingen/bedenkingen.

Bij het cta blok op bvb text+ Image en text 2 columns staan hier alle velden in het blok onder elkaar ipv de eerste twee naast elkaar:

Screenshot 2024-07-19 at 14 33 37

Hetzelfde bij het Text + Video blok hier kunnen we misschien optimaler zijn qua ruimte en het veld voor de placeholder image en de video position naast elkaar zetten:

Screenshot 2024-07-19 at 14 41 10

Eerder een frontend issue misschien, maar als ik de show in pop up aanvinktte kreeg de image ineens een achtergrond en deze is ook asymmetrisch.

Screenshot 2024-07-19 at 14 54 41

Bij het quote blok is het text veld eigenlijk een overviewTitle veld, waardoor de input gelimiteerd is maar kan je ook niet je volledige quote lezen als je een lange quote wilt toevoegen.

Screenshot 2024-07-19 at 14 56 01

Op het CTA blok is enkel de cta zelf verplicht, willen we hierbij misschien de titel of text ook verplicht maken?

Het table blok heeft trouwens geen enkel verplicht veld, ik zou hierbij toch op z'n minst het table veld verplichten.

Bij het text 2 columns blok is de titel en text van de eerste kolom verplicht en niks van de tweede kolom. Ik dacht toch dat we hierbij de text velden van beide kollomen vroeger verplichtten of was er een reden om dit niet te doen?

Ten slotte kan je bij het Form blok meerdere forms toevoegen maar wordt enkel de eerste getoond. Willen we dit limiteren tot max 1?

Screenshot 2024-07-19 at 15 01 30

@Numkil
Copy link
Contributor Author

Numkil commented Jul 19, 2024

@SarahOorts dankjewel van er zo grondig door te gaan. Ik zal je opmerkingen zo snel mogelijk opnemen in mijn PR

Copy link

sonarcloud bot commented Jul 22, 2024

@Numkil
Copy link
Contributor Author

Numkil commented Jul 22, 2024

@SarahOorts

Ik heb al je feedback verwerkt, overal de juiste requirements op gezet en waar mogelijk velden naast elkaar gezet.

De volgende 2 dingen heb ik niet aangepast.
Eerder een frontend issue misschien, maar als ik de show in pop up aanvinktte kreeg de image ineens een achtergrond en deze is ook asymmetrisch. Dit lijkt me bewust in de twig template te zitten en aangezien dit niets met de craft-5 upgrade te maken heeft laat ik het zo zijn.

Op het CTA blok is enkel de cta zelf verplicht, willen we hierbij misschien de titel of text ook verplicht maken? De twig template is zo gemaakt dat enkel het CTA veld verplicht is. Ik zou dit dus zo laten.

@HannahDeWachter HannahDeWachter merged commit 3c7f648 into develop Jul 22, 2024
3 checks passed
@HannahDeWachter HannahDeWachter deleted the reworkfieldscraft5 branch July 22, 2024 07:46
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.

3 participants