Issue #1058 improve tab and tab picker visibility

commitsovercoffee commented 9 months ago

Checklist

[x] For UI or styling changes, I have added a screenshot or gif showing before & after
[x] I have added a changeset

Description

Made below changes as mentioned in #1058

Updated style for tabs and tab picker.
Added a prop named activeTabColor for background color of tab picker button.

Before	After

Queries

I have made tabs to take full-width using flex-1 , was this the ask ?

After (with 2 tabs)	After (with 5 tabs)

Shall I add styling for dark-mode also ?

changeset-bot[bot] commented 9 months ago

🦋 Changeset detected

Latest commit: 6cdcbcb858b6693f04c5b66a8d0126428239788b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

| Name | Type | | ----------------------------- | ----- | | @evidence-dev/core-components | Patch | | @evidence-dev/evidence | Patch | | @evidence-dev/components | Patch | | evidence-test-environment | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel[bot] commented 9 months ago

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

Name	Status	Preview	Comments	Updated (UTC)
evidence-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 18, 2023 10:51pm

netlify[bot] commented 9 months ago

Deploy Preview for evidence-development-workspace ready!

Name	Link
Latest commit	6cdcbcb858b6693f04c5b66a8d0126428239788b
Latest deploy log	https://app.netlify.com/sites/evidence-development-workspace/deploys/64dff5d62860dd0007cef2f2
Deploy Preview	https://deploy-preview-1087--evidence-development-workspace.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

archiewood commented 9 months ago

Hey @commitsovercoffee,

Thanks so much for the PR. Functionally speaking this is excellent.

I see the styling pretty closely matches the example provided by another user in the original issue. I think that implementation is stylistically different from our project, so I have some suggestions on how we can match the Evidence "look and feel" a bit better.

Keep the body of the tabs as transparent, rather than grey
Maintain the same default tab header shape as before, but add the background colour when a tab is active
A slightly more subtle active background colour. Perhaps a lighter variant of the accent colour you have currently, or else a grey
Can we remove the {label} from the body of the tab. I see that the example provided in the issue had it, but I think for our implementation it would be more flexible for users if it wasn't included.

Once again - thank you for the contribution!

commitsovercoffee commented 9 months ago

Hey @archiewood, Please confirm the style that suits best.

Style 1 :

changes

For the border, I have updated the border shape with rounded-t. When you say Maintain the same default tab header shape, Should I keep the bottom border also ( similar to original design ) as shown below. 🤔

Style 2 :

bottom-blue

Style 3 :

active-bottom

To implement Style 2/3, a prop need to be exported to define bottom border color. 🧐

archiewood commented 9 months ago

Style 3 looks good to me!

Depending on how you implement, you could maybe just have one colour, with the background being a more transparent version of the line underneath?

commitsovercoffee commented 9 months ago

@archiewood

Description

Added a color prop which takes a 6 digit hex color code and sets it as bottom border color. Also it sets the background color for the active tab to the provided color with 0.1 opacity. If color is not provided it defaults to the original color.

Implementation

Passing string as a variable is not allowing to change the opacity with rgba(). But passing the color as an RGB array works. So I have written a function to convert the provided hex color code into an RGB array, which is passed as --activeTabColor, a css variable and is used with rgba() to vary opacity where needed.

without color prop	with color prop

Usage

## Tabs

<Tabs id="my-tabs" color="#96b6c5">  <!-- or "96b6c5" -->
    <Tab label="Tab 1">
        This is the content of Tab 1
    </Tab>
    <Tab label="Tab 2">
        This is the content of Tab 2
    </Tab>
</Tabs>

Limitations

value passed to color prop should be a 6 digit hex color code, which is a common way to describe color. If needed I can add rgb or hsl support but that seemed overkill hence mentioning here before adding it.

Let me know if any changes are needed 😊

archiewood commented 9 months ago

In all our other colour props, we support CSS name | hexademical | RGB | HSL

eg https://docs.evidence.dev/components/bar-chart#series

While I think CSS name is overkill (and maybe hard), if possible it would be great to support hex, rgb and HSL so users are not confused by this.

I believe you can implement transparency in all of these relatively easily:

hex: https://developer.mozilla.org/en-US/docs/Web/CSS/hex-color
RGB: as you've done
HSL: https://developer.mozilla.org/en-US/docs/Web/CSS/color_value/hsl

commitsovercoffee commented 9 months ago

Shall I follow the same approach though ? As I did for hex color input ?

<Tabs id="my-tabs" color="#96b6c5">

color prop takes only string input. That's okay right ?
a function verifies if it is a valid hex color. And if so it is passed as a css var to style the tab button.

So I'll just have to extend the function to handle rgb, hsl and css colors ?

archiewood commented 9 months ago

string is how we handle color elsewhere so I think fine to continue with that.

My thought would be just reading the start of the string to work out what type of code it is?

if (string starts with #) { 
    // hex 
    add 10% opacity by appending `1a`, `##fa1167 -> `##fa11671a`
}
else if (string starts with RGB or rbg) {
    // rgb 
    add 10% opacity via css
} 
else if (string starts with hsl) {
    // hsl
    add 10% opacity by appending to string
}

or something like that?

commitsovercoffee commented 9 months ago

@archiewood I have made the required changes. color prop now supports rgb, hsl and hex color formats.

<Tabs id="my-tabs" color="hsl(166, 26%, 48%)"/>
<Tabs id="my-tabs" color="rgb(160, 118, 249)"/>

hsl	rgb

Alternative Implementation Idea

Instead of passing a prop to <Tabs/> to define active color, we can also send custom css vars directly to a component as mentioned in the svelte tutorial, doing so we can directly use the passed color (including css colors) in the component style. But doing do I am unable to change the color opacity.

PS @ItsMeBrianD I have updated the changeset to patch :)

commitsovercoffee commented 9 months ago

@ItsMeBrianD I've made the changes. Thanks for the detailed feedback :)

ItsMeBrianD commented 9 months ago

@ItsMeBrianD I've made the changes. Thanks for the detailed feedback :)

Absolutely, glad to help; thank you for submitting the PR!

I'm going to kick off the tests once that last suggestion is added, and then I think we're good to go 😄

evidence-dev / evidence