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

[#44] Implement Blueprint Elements #46

Open
wants to merge 14 commits into
base: 5.x
Choose a base branch
from
Open

Conversation

joshuapease
Copy link
Contributor

@joshuapease joshuapease commented Sep 24, 2024

Overview:

  • Implements all of the items under "Elements" in Blueprint.
  • Configures Vite to copy SVGs from the /src/icons/ directory
  • Configures Craft with a filesystem and asset volume (so we can test image related features.
  • Installs a standalone "Viget Parts Kit" plugin (not 100% certain where we'll go with this, but I'm enjoying breaking up the base module features.

I've left comments on most of the components

Uses `vite-plugin-static-copy` to copy SVGs from src/icons

Macro inlines SVG files into page.
@joshuapease joshuapease marked this pull request as draft September 24, 2024 21:09
@@ -32,7 +32,7 @@ web_extra_exposed_ports:
web_extra_daemons:
# Run vite in a separate process
- name: 'vite'
command: 'npm install && npm run dev'
command: 'npm install && npm run build && npm run dev'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ensures that icons are present on start.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this works. I had been trying to work around this by either using src/public or the Vite HMR Plugin Copy plugin — this will work, but I think it won't catch additions or changes you make to that folder while working unless you ddev restart (since this is in a daemon and not a terminal tab where R would reload Vite) — right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah... you know what. I got thrown off by the name.

I thought it would only copy the public directory. But it seems like you can configure it to copy any directory?

I was hesitant to use the public directory for icons, because on some of our more complex sites, we occasionally need to reference icons from JS.

I'll put in a ticket to look into this more deeply.

CleanShot 2024-10-03 at 09 44 55@2x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@joshuapease joshuapease changed the title [#44] Base UI elements [#44] Implement Blueprint Elements Sep 25, 2024
@@ -30,3 +30,6 @@ CRAFT_SECURITY_KEY=
CRAFT_DEV_MODE=true
CRAFT_ALLOW_ADMIN_CHANGES=true
CRAFT_DISALLOW_ROBOTS=true

# Asset Settings
ASSET_FILESYSTEM=local
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the future, the env var would let you choose s3 or local depending on your environment or preference.

Future ticket for some default AWS setup

@@ -0,0 +1,107 @@
import plugin from 'tailwindcss/plugin'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exact copy of blueprint (only change was to make an ES Module)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exact copy of blueprint (only change was to make an ES Module)

@@ -1,8 +1,47 @@
import buttons from './config/tailwind/buttons'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mirroring the TW config of blueprint

@@ -0,0 +1,48 @@
{# @var string|null title #}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only change was to use Alpine for dismissible variant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Curious to hear everyone's thoughts on this.

It's using Imager X's power pack, but wraps that abstraction so if you ever change your image plugin, you don't have to rip it out everywhere.

Main goals for this component.

  1. Minimize the amount of transform config you need to spread around your project.
  2. Reduce the Imager X specific syntax in your project (would make it easier to switch from Imager X if needed).
  3. Use TW screen names in your sizes props.

ppimg() is doing some interesting stuff here.

Here's some further detail on the fillTransforms setting.

{% set minWidth = 320 %}
{% set maxWidth = 2200 %}

{{ image ? ppimg(
  image: image,
  transform: [
    minWidth,
    maxWidth, // [1.] Generate transforms between 320 and 2200
    {
      ratio: ratio,
    },
  ],
  params: {
    class: cx(class, 'w-full h-auto'),
    loading,
    fillTransforms: 'auto', // [2.] By using fillTransforms 'auto' we fill in between our min/max widths
    autoFillCount: 5, // [3.] Creates 5 transforms evenly spaced between our min/max widths
    allowUpscale: false,
    sizes: sizes,
  },
) }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Identical to Blueprint minus the ability to use slots.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Identical to Blueprint

I think over time our clients would benefit from the following:

  • lazy load the iframe
  • customizable thumbnail / play button

We also might want to consider using a plugin that parses URLs to get the video ID/Embed URL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simple wrapper around the svg function in Craft.

Inlines the SVG markup directly into the page.

Provides some icon sizing logic so you don't have to remember to pass size-xyz everywhere. Although there is the option to pass none as the size and use your own TW classes in unique scenarios.

Comment on lines +32 to +36
plugins: [
viteStaticCopy({
targets: [{ src: 'src/icons/**/*', dest: './assets/icons' }],
}),
],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how we handle icons on MW and it works pretty well.

Open to other suggestions too. I considered putting them in the public directory. But you loose the ability to bundle SVG with your JS.

@joshuapease joshuapease marked this pull request as ready for review September 27, 2024 00:02
Comment on lines +48 to +54
{% set screens = {
'sm': '(min-width: 640px)',
'md': '(min-width: 768px)',
'lg': '(min-width: 1024px)',
'xl': '(min-width: 1280px)',
'2xl': '(min-width: 1536px)',
} %}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

On some sites (like MW), we actually convert our TW config to JSON and then load it via PHP so we can get at these values.

But.... I'm of the impression lately that it a ton of complexity to save us 5-ish lines of code.

Do screen sizes really change all that much after a project has been started? This might be an acceptable DUPE

Copy link

@ten1seven ten1seven left a comment

Choose a reason for hiding this comment

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

Cool!

{% if title or description %}
<div class="flex flex-col gap-8">
{% if title %}
<h2 class="text-xl font-bold">{{ title }}</h2>

Choose a reason for hiding this comment

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

This H2 should could probably be a variable. I'm also wondering if H3 is a better default if a typical page structure looks like:

H1 Page Title
   |- H2 My Card List
      |- H3 Card Title
      |- H3 Card Title
      |- H3 Card Title

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call! I'll update in my next PR

{% if title or description %}
<div class="flex flex-col gap-8">
{% if title %}
<h2 class="text-xl font-bold">{{ title }}</h2>

Choose a reason for hiding this comment

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

Same question re: heading.

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