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

Button component refactor #216

Closed
endigo9740 opened this issue Sep 10, 2022 · 10 comments · Fixed by #233
Closed

Button component refactor #216

endigo9740 opened this issue Sep 10, 2022 · 10 comments · Fixed by #233
Assignees
Labels
documentation Improvements or additions to documentation enhancement New feature or request ready to test Ready to be tested for quality assurance.

Comments

@endigo9740
Copy link
Contributor

endigo9740 commented Sep 10, 2022

Given the complexity and breadth of buttons being used, I'm going to pull this into its own ticket. But this is related to:
#194

Todo:

  • Factor and cleanup code
  • Allow passing through actions/actionParams
  • Allow passing through svelte:prefetch and similar
  • Improve styling and variant code logic
  • Consider dropping size presets or re-evaluating how they work
  • Ensure all current standards and conventions are in place

Once ready, we'll need to update the various pages of the docs with the new changes.

@endigo9740 endigo9740 self-assigned this Sep 10, 2022
@endigo9740 endigo9740 added documentation Improvements or additions to documentation enhancement New feature or request labels Sep 10, 2022
@salisshuaibu11
Copy link
Contributor

Fill property is not taking effect on the button

@endigo9740 endigo9740 linked a pull request Sep 15, 2022 that will close this issue
@endigo9740
Copy link
Contributor Author

endigo9740 commented Sep 16, 2022

I have a proposal for buttons. Put frankly, I think our dependence on a component is not the best way to handle these elements. Instead, we should opt for a route not unlike what we're doing with form inputs - that is, lean more into Tailwind CSS and the utility class approach. However, unlike forms, this won't require an extra plugin. Just an "add-on" stylesheet like typography.css.

Reference:
https://skeleton.brainandbonesllc.com/guides/styling

I know this is a kind of wild idea, so I've built a draft PR as a proof of concept to share. Please give it a try!
#233

I've implemented the changes at the top of the Components > Buttons doc page.

Here's how it works:

  • We implement a new file in /lib/styles/buttons.css to house the new styles.
  • This would need to be imported into the root layout one time, like other add-ons.
  • Then, everything works like standard Tailwind CSS utility classes

First off, here's how it looks. Visually users won't be able to tell the difference. Hover and click animations remain in tact as well!

Screen Shot 2022-09-15 at 6 21 43 PM

Screen Shot 2022-09-15 at 6 22 11 PM

Here's a screenshot of the markup. View it here.

Screen Shot 2022-09-15 at 6 23 11 PM

Here's what the stylesheet looks like. View it here.

Screen Shot 2022-09-15 at 6 35 28 PM

Finally, here's the important part. Comparing the component (left) to CSS (right). A few notes:

  • You do not have to repeatedly import the button class on each page or within components
  • The markup overall is a bit slimmer because classes can be grouped, rather than having to list a ton of key/value props
  • No Svelte fragment needed, just wrap children with spans so flexbox can work
  • CSS buttons are infinitely customizable, because it's all just utility classes
  • By using native elements, key features like svelte:prefetch are possible (note I wrote my example in the old syntax; the docs are current)
  • I was able to introduce a whole new type of button (icon buttons) with this method. This would have been challenging with the component

Screen Shot 2022-09-15 at 6 32 57 PM

Additionally, your first concern may be whether or not this increases the bundle size. The short answer is - NO. Given the way the Tailwind compiler works, all those preset classes we add in the Button component would have been inserted into the final bundle. So this is really no different. The big difference comes from the fact the Button styles are 100% optional. If users just want to create their simple button styles, they don't have to use ANY of this. Additionally, I think it may be possible to migrate these styles into our TW plugin - if we do this then tree shaking would be possible! (though don't expect that right away; we need to grow into this)

One final note - I am overriding the global styles of buttons very slightly. After importing the button.css file, I would add something like this to my global css. The default buttons use the inherent font thickness and square corners:

Screen Shot 2022-09-15 at 6 52 26 PM

But overall, this looks to work better than what the Button component does. Less imports, more customization, and the ability to use SvelteKit-only features.

Thoughts? Concerns? Really I want folks to poke holes in this if I've missed something obvious.

@endigo9740 endigo9740 pinned this issue Sep 16, 2022
@endigo9740
Copy link
Contributor Author

endigo9740 commented Sep 16, 2022

I've come up with a solution for how to present the new CSS buttons compare to the prior Svelte Components. The inspiration comes from how we're using Forms - where we just use Tailwind and CSS to style these. Following this lead, we segment things like so:

  • Tailwind Elements - native elements that are styled via our add-on stylesheets using Tailwind
  • Svelte Component - these are true Svelte components

@niktek This makes your earlier idea possible - providing a means to create Tailwind styled elements. This gives us a place to add everything from hero banners to full page layouts, similar to Flowbite and Tailwind UI:

However, rather than having to pair multiple libraries together, you get it this as an all-in-one kit. In fact, I like this idea so much I'm considering changing Skeleton's description from a "Svelte + Tailwind UI component library" to a "Svelte + Tailwind UI toolkit". I'm really excited about this prospect!

The new segmentation in the left sidebar navigation, plus the new CSS Buttons page:

Screen Shot 2022-09-16 at 2 17 00 PM

Here's what the interactive sandbox example looks like. Note the dynamic code block snippets at the bottom.

btn-sandbox

Likewise variants are still avilable:

Screen Shot 2022-09-16 at 2 33 57 PM

The draft PR has been updated if you want to pull and review.

@salisshuaibu11
Copy link
Contributor

This is indeed a really good idea I think, I have come across this in one svelte repository you can check it out:

https:/AgnosticUI/agnosticui/blob/master/agnostic-svelte/src/lib/components/Button/Button.svelte

@endigo9740
Copy link
Contributor Author

endigo9740 commented Sep 17, 2022

It's a similar idea, but they appear to still be providing a Button component. With the change above we wouldn't. You would use native browser button/anchor tags and then utilize our utility classes to style the buttons as you wish. It's a purely CSS solution, not unlike something like Flowbite.

There would be zero benefit to providing a component in our use case, and in fact, that would limit features if we did (such as the Svelte prefetch attribute). We just let button be buttons and anchors be anchors.

@salisshuaibu11
Copy link
Contributor

Yeah understood, just like the way we are utilizing tailwind classes. So we will not have any Button component just like the way we don't have it for other form elements.

@endigo9740
Copy link
Contributor Author

Awesome, well thanks for checking out out @salisshuaibu11! My plan will be to move forward with implementing this at the de facto option in the Docs site today. But I'll hold off on deleting the Button component for any stragglers that still want to test it before it's merged.

@endigo9740
Copy link
Contributor Author

endigo9740 commented Sep 17, 2022

All new updates have been pushed to the PR. This updated has changed from a proposal/draft to a pending PR. I think the idea is solid and the execution is working as expected.

The latest updates make the process a bit simpler than the earlier drafts mentioned above. You're no longer required to supply a size for every button instance, which means converting buttons that utilized variants (most on our docs site) is very straight forward:

<!-- Before - using a component -->
<Button variant="filled-accent" href="/guides/styling">Foo</Button>

<!-- After - using the classes -->
<a class="btn btn-filled-accent" href="/guides/styling">Foo</a>

I was able to replace about 125 instances of the button in our docs in probably 30 minutes.

In addition to dropping the extra import on every page that uses buttons, this actually makes it MUCH easier to handle styling for components with embedded buttons. Such as the Paginator or Stepper. Instead of having to find a means to pass through a series of props, we just pass arbitrary classes to style the native <button> element. It's great, a huge improvement over the prior methods!

All that's left at this point is to nuke and remove the Button component from the project and then merge, unless anyone has any other ideas to suggest that is! ETA for this will be in the next 24-48 hours.

NOTE: I've also migrated the Core, Typography, and Forms section under "Tailwind Elements" and updated the docs accordingly.

@endigo9740
Copy link
Contributor Author

These changes have been merged in to the dev branch, but this ticket will remain open so QA testing can continue.

@endigo9740 endigo9740 added the ready to test Ready to be tested for quality assurance. label Sep 19, 2022
@endigo9740
Copy link
Contributor Author

Released!

@endigo9740 endigo9740 unpinned this issue Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request ready to test Ready to be tested for quality assurance.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants