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

Feature/accept all colours #506

Merged
merged 25 commits into from Jan 19, 2021

Conversation

shwinnn
Copy link
Collaborator

@shwinnn shwinnn commented Jan 11, 2021

Allows the user to pass any valid css colour to the "color" property of components, as well as the bootstrap built in colours as before.

@shwinnn shwinnn requested a review from tcbegley January 11, 2021 17:36
@shwinnn
Copy link
Collaborator Author

shwinnn commented Jan 12, 2021

Added some tests.

Copy link
Collaborator

@tcbegley tcbegley 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 looking good. A few comments:

  1. Can we kill the package-lock.json file? The diff is unusually large (were you using Yarn?) and I normally wait until making a release to update it in any case to minimise noise in the commit history.
  2. The fill colour on hover with dbc.Button(..., outline=True) doesn't get updated, it stays as secondary (default). This is probably hard to fix because you can't set hover styles inline.
  3. Related to last point, should we perhaps steer people towards default colours in Button? It's not just the outline style that suffers, but also the hover effects for regular buttons get destroyed by the override here. I basically am unsure we should make it easier for people to make their app look worse 😆

Anyway, other than that it's all looking pretty good! Here's the app i was testing with for reference. Note that I chose the colours randomly for testing haha.

import dash
import dash_bootstrap_components as dbc
import dash_html_components as html

app = dash.Dash(external_stylesheets=[dbc.themes.BOOTSTRAP])

app.layout = html.Div(
    [
        dbc.NavbarSimple(brand="Navbar", color="red"),
        dbc.Navbar("Hello", color="blue"),
        dbc.Container(
            [
                dbc.Alert("Hello", color="chartreuse"),
                dbc.Button("Hi", color="rebeccapurple"),
                dbc.Button("There", outline=True, color="cyan"),
                dbc.Badge("Badge", color="magenta"),
                dbc.Label("Testing", color="orange"),
                dbc.Progress(
                    color="chocolate", value=50, striped=True, animated=True
                ),
                dbc.Spinner(color="darkviolet"),
                dbc.Card("This is a card", body=True, color="deepskyblue"),
                dbc.DropdownMenu(
                    [dbc.DropdownMenuItem("Item 1")],
                    label="Toggle",
                    color="lightcoral",
                ),
                dbc.FormText("Text", color="lightsteelblue"),
                dbc.ListGroup(
                    [
                        dbc.ListGroupItem("Item 1", color="fuchsia"),
                        dbc.ListGroupItem(
                            dbc.ListGroupItemText("Text", color="chocolate")
                        ),
                    ]
                ),
            ],
            className="p-5",
        ),
    ]
)

if __name__ == "__main__":
    app.run_server(debug=True)

@shwinnn
Copy link
Collaborator Author

shwinnn commented Jan 13, 2021

I've sorted out the problems with hovering at the cost of 2 new requirements. Bundle size only goes up to 302KB though. What do you think?

@shwinnn
Copy link
Collaborator Author

shwinnn commented Jan 14, 2021

Reverted the hover colours, button is back to only allowing bootstrap contextual colours.

Copy link
Collaborator

@tcbegley tcbegley left a comment

Choose a reason for hiding this comment

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

This looks great. If you could just put the Button test back in (looks like it got deleted in the back and forth on that component) I think this is ready to go.

src/components/__tests__/Button.test.js Outdated Show resolved Hide resolved
@shwinnn
Copy link
Collaborator Author

shwinnn commented Jan 19, 2021

Fixed!

Copy link
Collaborator

@tcbegley tcbegley left a comment

Choose a reason for hiding this comment

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

Thanks @shwinnn!

@tcbegley tcbegley merged commit c852db9 into facultyai:master Jan 19, 2021
This was referenced Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants