Rules List #340

merowin · 2024-11-22T21:05:55Z

This adds a new view that is linked to in the NavBar and has links to the rules page for variants with a (detailed) rules description.
Also I've added the demo board to the rules view as proposed in #112
I wasn't able to make sure that the demo board fits, so I made its container scrollable on overflow.

Rules List

Rules Page

benjaminpjones · 2024-11-23T00:44:20Z

Very nice! These go great together, and I'm glad we're finally linking them from the main page.

I haven't had a chance to look at the code, but some visual thoughts:

Text is a bit wide IMO.
- I searched around, and there is no one-size-fits-all approach. However, most readability guidelines fall in the 45-90 characters range, whereas this is about 130 characters long.
- Narrowing the text may also solve the space issue for the demo board as well.
On mobile, text could use some margin - I think it's like 3 px now, I'd recommend at least 8px
- Compare Medium.com, which gives 24px on mobile
- This might actually be some feedback for the website in general (config form also seems a bit cramped without margin), so we don't need to address in this PR

benjaminpjones

LGTM, thanks!

benjaminpjones · 2024-11-23T12:52:59Z

packages/vue-client/src/assets/base.css

@@ -93,7 +93,6 @@
  }
 }

-/* TODO: Check why this is here, can we remove this? */


Does this mean you figured it out?

I've tested around a bit, and realised 2 things:
1.) We can view these as changes to the "default styles" of css, and I think these are good.
2.) Components can still overwrite these styles if need be.

I added this TODO when I added some detailed rules, and the markdown is displayed differently than I expected. For the rules it might still be worth to overwrite this, but it's fine for now imo.

Makes sense, thanks for the explanation!

benjaminpjones · 2024-11-23T12:55:57Z

packages/vue-client/src/utils/format-utils.ts

@@ -0,0 +1,7 @@
+export function toUpperCaseFirstLetter(string: string) {


Aside: Not a priority, but I feel like we should have some UI-friendly strings defined. The variant names are not really designed to be shown to the user.

JonKo314 · 2024-11-23T18:30:48Z

packages/vue-client/src/views/RulesListView.vue

+        v-for="variant in variantsWithRulesDescription"
+        :key="variant"
+        class="rules-link"
+        v-bind:to="{ name: 'rules', params: { variant: variant } }"


FYI: Because of the inconsistency between :key and v-bind:to I want to point out, that : is a shorthand for v-bind:. See Vue's v-bind directive

We might consider a lint rule (vue/v-bind-style or vue/vue3-strongly-recommended). We're pretty inconsistent across the codebase unfortunately.

I've changed it, will try to be more consistent! 😄 Thanks!

JonKo314 · 2024-11-23T18:40:41Z

packages/vue-client/src/views/RulesView.vue

  font-family: "helvetica", "arial", "sans-serif";
 }
+
+.grid-page-layout {


Is this the class that is used in the demo view? That could be clarified with a comment maybe

Doesn't <style scoped> prevent the style from getting used in DemoView anyway (without deep selector)?

Yeah this is what we renamed pageWrapper to. AFAIK with scoped it should only be applied to this component, and not its child components. see https://vue-loader.vuejs.org/guide/scoped-css.html

Oh, I missed that the class was applied in this file too (I just saw it being deleted, overlooked it in the added part).

But:

With scoped, the parent component's styles will not leak into child components. However, a child component's root node will be affected by both the parent's scoped CSS and the child's scoped CSS. This is by design so that the parent can style the child root element for layout purposes.
SFC CSS Features | Vue.js

So I think it gets applied to both?

I'm not sure what the documentation means with root node. Does this mean the DOM tree?
But I tried it, and indeed the style is applied to both in this case. Good to know!
I've pushed a change that makes sure it's only applied to the rules page layout in this case.

DemoView has <template><div class="grid-page-layout"> ... </div></template>, I think the template is ignored, so the <div class="grid-page-layout"> is the root node.

Didn't know about that, thanks for explaining!

merowin · 2024-11-23T18:54:47Z

* Text is a bit wide  IMO.
  
  * I searched around, and there is no one-size-fits-all approach.  However, most readability guidelines fall in the 45-90 characters range, whereas this is about 130 characters long.
  * Narrowing the text may also solve the space issue for the demo board as well.

* On mobile, text could use some margin - I think it's like 3 px now, I'd recommend at least 8px
  
  * Compare Medium.com, which gives 24px on mobile
  * This might actually be some feedback for the website in general (config form also seems a bit cramped without margin), so we don't need to address in this PR

Thanks for the feedback! I've increased the padding on mobile to 12px (for all pages using grid-page-layout), and added some limits for the width of the text column.

merowin added 4 commits November 21, 2024 09:28

add demo board to rules page

0c3445b

Merge branch 'main' into add_demo_board_to_rules_page

75d4456

increase max width in rules page, catch typos in variant prop

cb0e21a

add rules list view, link to it in nav bar

7bdbab3

benjaminpjones approved these changes Nov 23, 2024

View reviewed changes

JonKo314 reviewed Nov 23, 2024

View reviewed changes

merowin added 2 commits November 23, 2024 19:49

add padding on mobile, limit text size in rules page

681c4ba

shorthand v-bind

1b0dbc9

make sure style is not applied to demo board layout

67cdbeb

JonKo314 approved these changes Nov 23, 2024

View reviewed changes

merowin merged commit 90751b8 into main Nov 23, 2024
3 checks passed

merowin deleted the add_demo_board_to_rules_page branch November 24, 2024 16:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rules List #340

Rules List #340

merowin commented Nov 22, 2024 •

edited

Loading

benjaminpjones commented Nov 23, 2024 •

edited

Loading

benjaminpjones left a comment

benjaminpjones Nov 23, 2024

merowin Nov 23, 2024

benjaminpjones Nov 23, 2024

benjaminpjones Nov 23, 2024 •

edited

Loading

JonKo314 Nov 23, 2024

benjaminpjones Nov 23, 2024

merowin Nov 23, 2024 •

edited

Loading

JonKo314 Nov 23, 2024

benjaminpjones Nov 23, 2024

merowin Nov 23, 2024

JonKo314 Nov 23, 2024

merowin Nov 23, 2024

JonKo314 Nov 23, 2024

benjaminpjones Nov 24, 2024

merowin commented Nov 23, 2024

		@@ -0,0 +1,7 @@
		export function toUpperCaseFirstLetter(string: string) {

Rules List #340

Rules List #340

Conversation

merowin commented Nov 22, 2024 • edited Loading

Rules List

Rules Page

benjaminpjones commented Nov 23, 2024 • edited Loading

benjaminpjones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminpjones Nov 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merowin Nov 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merowin commented Nov 23, 2024

merowin commented Nov 22, 2024 •

edited

Loading

benjaminpjones commented Nov 23, 2024 •

edited

Loading

benjaminpjones Nov 23, 2024 •

edited

Loading

merowin Nov 23, 2024 •

edited

Loading