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

Add Button Playbook #5

Open
wants to merge 5 commits into
base: next
Choose a base branch
from
Open

Conversation

lizziekoshelev
Copy link

@lizziekoshelev lizziekoshelev commented Mar 23, 2023

Import the following playbook to our design system docs:
https://www.notion.so/teamshares/Button-Playbook-3341236385ca44ccac0eb81deb72720b?pvs=4

Preview it by clicking "Visit Preview" below and then "Recipes" > "Buttons"

@vercel
Copy link

vercel bot commented Mar 23, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
shoelace ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 18, 2023 11:01am

Copy link
Collaborator

@kdonovan kdonovan left a comment

Choose a reason for hiding this comment

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

Other than naming and a formatting tweak the rough plan looks good to me! Will defer to Daross on checking the functionality of the proposed replacements. :)

docs/teamshares/recipes/buttons.md Outdated Show resolved Hide resolved
docs/teamshares/recipes/buttons.md Outdated Show resolved Hide resolved
@@ -2,4 +2,5 @@

- ## [Protips](/teamshares/recipes/protips.md)
- ## [Rails UJS](/teamshares/recipes/rails-ujs.md)
- ## [Converting Button View Component To Shoelace](/teamshares/recipes/buttons.md)
Copy link
Author

Choose a reason for hiding this comment

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

@kdonovan updated naming here as well, do you think it's worth to change the file name? Or keep since this is the only recipe book for buttons

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, let's just tweak it to e.g. shared-ui-buttons.md 👍

Choose a reason for hiding this comment

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

Looking at this, I'm realizing that we should probably:
a. Make the link titles short and pithy so they fit easily in the sidebar, and
b. Add a little description line under each of them on this recipes.md page
In this case, I'd make the title just Converting SharedUI Buttons, and then under that a line that says Recipes for replacing various SharedUI::ButtonComponent usages with sl-button, to make it super explicit.

Copy link
Collaborator

@kdonovan kdonovan left a comment

Choose a reason for hiding this comment

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

Looks alright to me, once @CrookedGrin signs off on the contents of the replacements (and... maybe this src/components/tab-group/tab-group.test.ts test is failing in webkit?)

@@ -2,4 +2,5 @@

- ## [Protips](/teamshares/recipes/protips.md)
- ## [Rails UJS](/teamshares/recipes/rails-ujs.md)
- ## [Converting Button View Component To Shoelace](/teamshares/recipes/buttons.md)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, let's just tweak it to e.g. shared-ui-buttons.md 👍

Copy link

@CrookedGrin CrookedGrin left a comment

Choose a reason for hiding this comment

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

This is great content! A bunch of minor formatting requests and some additional explanatory context, and this will be good to go.

@@ -2,4 +2,5 @@

- ## [Protips](/teamshares/recipes/protips.md)
- ## [Rails UJS](/teamshares/recipes/rails-ujs.md)
- ## [Converting Button View Component To Shoelace](/teamshares/recipes/buttons.md)

Choose a reason for hiding this comment

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

Looking at this, I'm realizing that we should probably:
a. Make the link titles short and pithy so they fit easily in the sidebar, and
b. Add a little description line under each of them on this recipes.md page
In this case, I'd make the title just Converting SharedUI Buttons, and then under that a line that says Recipes for replacing various SharedUI::ButtonComponent usages with sl-button, to make it super explicit.


## 1. Link button

> 💡 Hint: these are buttons marked with `external: true` or marked with `render_as_link: true`

Choose a reason for hiding this comment

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

I like these little blockquote hints. The italicized emoji looks a bit weird, but we can figure out formatting for that later.

I think some of the hints here should just be paragraphs of explanation, in which I'd err on the side of over-explaining. Here, I would just make this a paragraph that says "SharedUI::ButtonComponents with external: true or render_as_link: true are rendered as HTML <a> tags. To get a similar effect in Shoelace, use sl-button with an href attribute."

| Schedule prep session
```

> 💡 Hint: use `target="_blank"` to open the link in a new tab

Choose a reason for hiding this comment

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

And then short little tips like this can remain as "hints".


## 2. Submit button

> 💡 Hint: these are commonly used with simpleform and have `submit: true`

Choose a reason for hiding this comment

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

Here again I would lengthen this explanation a bit and make it just a regular paragraph


### Before

```

Choose a reason for hiding this comment

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

For all of these code blocks, you can get the right formatting via

```pug


### After (this example uses a content tag)

```jsx

Choose a reason for hiding this comment

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

pug and multiple-lines here as well

```

## 5. Modal button

Choose a reason for hiding this comment

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

I think the modals in Buyout are a bit different from the ones in OS, so we may need to update this to make that clear.

Choose a reason for hiding this comment

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

Would also add a little context about what we're talking about here, i.e., "To replace SharedUI::ButtonComponents that are used to launch modals..."


> 💡 For more information check out [our Rails UJS recipe page](https://design.teamshares.com/#/teamshares/recipes/rails-ujs)

```jsx

Choose a reason for hiding this comment

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

pug


## 6. Internal link/path with no explicit associated form

> 💡 Hint: for :get requests you can just use a link button above, for :delete, :post, :patch, etc. we now have a ButtonTo view component that behaves as rails’ [button_to](https://apidock.com/rails/ActionView/Helpers/UrlHelper/button_to)

Choose a reason for hiding this comment

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

Make sure to put the :get, :post, ButtonTo etc in code format for readability


### After

```jsx

Choose a reason for hiding this comment

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

pug

Copy link

@ebabian ebabian left a comment

Choose a reason for hiding this comment

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

Nice work!! This looks great. I noticed in the Recipes side nav section, the link to this playbook doesn't route to the correct page, it only works from the actual recipes page. Heres a video demoing the issue:

Screen.Recording.2023-04-19.at.9.31.58.AM.mov

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.

4 participants