feat(#5): Add Button component

[ynotdraw]

🚀 Description

This PR introduces the Button component.

Closes #5

---

❓ Outstanding Questions

• Should we provide a fractal page object for the Button component?
• Add template tag and convert the test to .gts in a follow up PR or pair with someone next week

---

🔬 How to Test

CI
CI (Actions) should all be green 🟢 . CI tests linting and tests!

Docs App

Pull down this repo and run the following:

pnpm i
cd docs-app
pnpm start

Go to the docs page and behold all of their glory.

---

📸 Images/Videos of Functionality

Quick scroll-through of the docs-app page

demo.mov

Image of docs-app page

[ynotdraw]

Similar note here as above

[ynotdraw]

The icon I used in this example is from an MIT-licensed open source library I built. We can replace it at any time, as we see fit.

[ynotdraw]

Same comment as above in regards to icons here

[ynotdraw]

This Button instance actually has a collision with the Button component from @crowdstrike/ember-oss-docs. For now, we can continue to write docs, but be aware you may hit this if there are components named the same there.

The eventual goal is to:

• Release this as a package
• Update ember-oss-docs to use this repo

A bit of a 🐔 and 🥚 problem

[ynotdraw]

I am not a writer, so appreciate any feedback in this entire document. I did my best tho ☺️

[NullVoxPopuli]

sounds good to me 🙃

[NullVoxPopuli]

Unlike our internal docs, which focus on all disciplines at once, these "core" docs, are primarily focused on developers.

[simonihmig]

Nice work! 👍

I do have a question about the role of the component though, see my inline comment...

[simonihmig]

I might be missing context here, like it could be that this was designed exactly like implemented here for good reasons, but I am not sure I fully understand the design choices here, in terms of what assumptions about the passed content this component needs to make, and basically how opinionated it wants to be...

Like do we have strong opinions here (compared to the consumer side) about e.g. how a button in loading state should look like? Like you could have a strong opinion by mandating that a loading button always need to have a specific (according to our design system) loading spinner icon. Which we don't have here. Or you could be very un-opinionated by delegating all content choices to the consumer. But this is somewhat in the middle, by delegating a good amount of things to the consumer by yielding, but still having a strong opinion that the default button content should be in DOM, but invisible (to sighted users).

For example. this could be how an un-opinionated button could work:

Suggested change

	{{#if @isLoading}}
	{{yield to="loading"}}
	<span class="sr-only">{{yield}}</span>
	{{else if @isDisabled}}
	<span class="flex items-center gap-x-2">
	{{yield}}
	{{yield to="disabled"}}
	</span>
	{{else}}
	{{yield}}
	{{/if}}
	{{#if @isLoading}}
	{{yield to="loading"}}
	{{else if @isDisabled}}
	{{yield to="disabled"}}
	{{else}}
	{{yield}}
	{{/if}}

I am not necessarily advocating for that, I am just basically about the role this button component should play, and how opinionated it should be? Like you could as well argue to bake more opinions into it (that's what a design system does), like in the example mentioned above about having a loading spinner baked into it.

Also, I think the use of multiple {{yield}} could get confusing here. Like I probably wouldn't have expected that the content of a disabled button is the combination of both <:default> and <:disabled> blocks, and (again an opinion here) that the disabled block comes after the default one.

[NullVoxPopuli]

I think the opinionatedness is good, because Buttons tend to be a bit complicated, and providing accurate styling for them (especially when it comes to the position of an icon), as non trivial, and consumers of Buttons often get it wrong.
So, whenever we have the possibility for a default icon to be present, I think it's good that we have a flex gap-x-2 wrapper in there.

This also forces the consumer of the Button to be mindful of their own design language, in the Toucan design system does not want icons on the right-hand-side of buttons, because that is used exclusively for "status" (disabled, loading, etc).

For the first block, where we also have an sr-only class, I think is a good idea, because people are not good at accessibility by default, so the more we can do for them, the better.

These conventions are part of the design system as a whole, and less so "just things we do", it's very much in the same spirit of how Material handles the label and underline on their inputs -- the features they describe are required, otherwise (and the same applies here), we'd only be providing "light styled basic elements".

What will be important to mention in our docs for this button, however, is these behaviors happening via yield, with lots of examples.
We need to, in our docs for this Button, explain every use case, else, I agree folks won't understand -- but we want them to understand, and documentation can help with the context around these button decisions.

[simonihmig]

I think the opinionatedness is good

So, as I tried to explain above, I was not arguing pro or contra opinionatedness, I was mostly asking how opinionated we want to be. And I feel this is currently somewhere in the middle of both worlds, and not sure if it is really well balanced this way...

For the first block, where we also have an sr-only class, I think is a good idea, because people are not good at accessibility by default, so the more we can do for them, the better.

To take this as an example: yes, the component now has an opinion about buttons in a loading state, in that the default label is now hidden for sighted users. But it does not have an opinion on what to show to sighted users instead, as it delegates that to the consumer by {{yield to="loading"}}.

So what happens when I do <Button @loading={{true}}>Save</Button>?
AFAICT it would render an empty button, no (visual) text, no icon, right? Is that expected?

So if we want to be opinionated, shouldn't we bake the looks of a loading button into the component, as it is part of a design system that probably has an opinion on how a loading button should look like, e.g. by implicitly showing a toucan loading spinner as a replacement for the default text?

[NullVoxPopuli]

But it does not have an opinion on what to show to sighted users instead

this is, in part, because we allow any icon set to be used.
Internally we show an icon only, but a consumer may want to use their own icon, or their own text.

I would like to tighten this up in a major / breaking change release, but we need to split our icon set before we can do that.

<Button @loading={{true}}>Save</Button>?
AFAICT it would render an empty button, no (visual) text, no icon, right? Is that expected?

yeah, for this reason, we'd want to either:

• provide lints that go along with this library (I'm doing the same for ember-resources for anti-footgun stuff, here: Create eslint plugin NullVoxPopuli/ember-resources#709)
• open source our base icons and force their usage (this would mean to build a full app, folks would have a mix of icon sets)
  • our base icon set is not presently open sourced due to scope-explosion in how everyone wanted to automated how icon exports occur from Figma (automatically creating v2 addon packages based on the Figma "Frame") -- it's a lot of "cron" work, but still probably worth doing

[ynotdraw]

provide lints that go along with this library (I'm doing the same for ember-resources for anti-footgun stuff, here: NullVoxPopuli/ember-resources#709)

I really love this idea 😄

[NullVoxPopuli]

re the outstanding questions:

Should we provide a fractal page object for the Button component?

I don't know that it necessarily needs to be a fractal page object, but it's probably a good idea to standardize on those.
fractal page object solves the problem of nesting helpers and querying elements, which a Button doesn't really have that problem -- future components certainly would though.

Add template tag and convert the test to .gts in a follow up PR or pair with someone next week

I personally would. I very much do not like working in the old format of tests these days 🙃
We also need a total of 200 repos using this format before GitHub will consider a PR that adds native syntax highlighting support to the Web UI. We're def still in early adopter territory -- but only because "the format", does not have a single location for docs, and linting has a bug or two, etc.

[ynotdraw]

Thanks y'all for the patience and assists! I believe I've covered all of the feedback and issues and this should be safe to re-review now! 🙇

[simonihmig]

Approved for my part, but please have a look at the comment, and see if that needs to be addressed.

Nice work!

[simonihmig]

I haven't really dived deep into the more special CI setup you and @NullVoxPopuli have in use for GH repos here (forms is still on blueprint defaults), so leaving the final assessment to you guys. But it seems to me if the intend was to do less by reusing build artefacts across jobs, then we loose much (all?) of this benefit when forcing to install all dependencies again, don't we?

[ynotdraw]

Good point. Some background:

I was talking to @NullVoxPopuli about this yesterday as I was hitting an issue where the lint step (which runs glint) was failing for the test-app. We essentially need the dist directory to be able to successfully run lint, or else we get type errors when running glint. Here's an example: https://github.com/CrowdStrike/ember-toucan-core/actions/runs/4009103591

Talking to @NullVoxPopuli , the reason we need to re-install dependencies is due to pnpm. We need to re-run pnpm i to re-sync the injected links. We do the same thing over in ember-headless-table (https://github.com/CrowdStrike/ember-headless-table/blob/f08ff968d862d2267a3b2f6269116806549d51b0/.github/actions/download-built-package/action.yml#L11).

I do agree with you though, that this feels like we add additional time to each step using this action by forcing dependencies to re-install. Maybe there's an optimization we can make in the future though? I'm also completely new to pnpm, so I don't have much context other than these convos! 🙇

[simonihmig]

Oh, I see. I guess this is this inject thing that Preston told me also about...

Yeah, ok. Unfortunate, but hopefully this will be sorted out in the future somehow...

[NullVoxPopuli]

Yeah, hoping we can work out a plan of attack with @zkochan (pnpm owner) with making injected auto-sync.
My current best idea is to analyze package.json#files/exports for folders, and symlink those, so that files automatically sync, and we don't need the extra pnpm i.

Related:

• Make depedenciesMeta inject by default for workspace packages? pnpm/pnpm#4965