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

Altering theme for more subtle alerts / labels / buttons #798

Merged
merged 3 commits into from
Aug 2, 2016

Conversation

mistercrunch
Copy link
Member

@mistercrunch mistercrunch commented Jul 21, 2016

@ascott @williaster The alerts were rough on my eyes and desire a more sober theme (at least for now).

I threw in a /caravel/theme/ endpoint to make it easier to iterate on css/less/themes.

screen shot 2016-07-21 at 9 32 27 am
screen shot 2016-07-21 at 9 32 20 am
screen shot 2016-07-21 at 9 32 15 am

@coveralls
Copy link

coveralls commented Jul 21, 2016

Coverage Status

Coverage decreased (-0.02%) to 81.166% when pulling a02840a on mistercrunch:theme-pasterl into fa0497d on airbnb:master.




<div id="global-zeroclipboard-html-bridge" class="global-zeroclipboard-container" style="position: absolute; left: 0px; top: -9999px; width: 15px; height: 15px; z-index: 999999999;" title="" data-original-title="Copy to clipboard"> <object classid="clsid:d27cdb6e-ae6d-11cf-96b8-444553540000" id="global-zeroclipboard-flash-bridge" width="100%" height="100%"> <param name="movie" value="/assets/flash/ZeroClipboard.swf?noCache=1469113730367"> <param name="allowScriptAccess" value="sameDomain"> <param name="scale" value="exactfit"> <param name="loop" value="false"> <param name="menu" value="false"> <param name="quality" value="best"> <param name="bgcolor" value="#ffffff"> <param name="wmode" value="transparent"> <param name="flashvars" value="trustedOrigins=getbootstrap.com%2C%2F%2Fgetbootstrap.com%2Chttps%3A%2F%2Fgetbootstrap.com"> <embed src="/assets/flash/ZeroClipboard.swf?noCache=1469113730367" loop="false" menu="false" quality="best" bgcolor="#ffffff" width="100%" height="100%" name="global-zeroclipboard-flash-bridge" allowscriptaccess="sameDomain" allowfullscreen="false" type="application/x-shockwave-flash" wmode="transparent" pluginspage="http://www.macromedia.com/go/getflashplayer" flashvars="trustedOrigins=getbootstrap.com%2C%2F%2Fgetbootstrap.com%2Chttps%3A%2F%2Fgetbootstrap.com" scale="exactfit"> </object></div><svg xmlns="http://www.w3.org/2000/svg" width="1140" height="500" viewBox="0 0 1140 500" preserveAspectRatio="none" style="display: none; visibility: hidden; position: absolute; top: -100%; left: -100%;"><defs><style type="text/css"></style></defs><text x="0" y="57" style="font-weight:bold;font-size:57pt;font-family:Arial, Helvetica, Open Sans, sans-serif">Thirdslide</text></svg></body>
Copy link

Choose a reason for hiding this comment

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

what does this line do?

Copy link
Contributor

Choose a reason for hiding this comment

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

was also curious about this

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure, it came from wget-ing the bootstrap theme demo page... I'm taking it out

@ascott
Copy link

ascott commented Jul 21, 2016

i like the idea of a theme template to test our css changes, and maybe also add other components to in the future, but i think it would be nice to remove some of the bootstrap specific text. also looks like it's throwing some errors we should fix.

could we also remove all the links to bootstrap docs?

screenshot 2016-07-21 11 14 40

maybe we could just include the elements we use in caravel, like menus, buttons, alerts, tables? simplify 🎉

@ascott
Copy link

ascott commented Jul 21, 2016

do we want the query button to use the primary color here? i think the light grey looks better and doesn't draw too much attention.

before:
screenshot 2016-07-21 12 56 17

after:
screenshot 2016-07-21 12 55 47

looks like we need to treat links in alerts differently in alerts with these new lighter colors:
screenshot 2016-07-21 12 53 21

@williaster
Copy link
Contributor

I like the idea of the theme test page as well but agree it could be nice to simplify and include only the components we utilize in Caravel. alanna as we move to React do you think it would make sense to use storybook for this type of thing as well?

I like the muted / grey query button as well. I think once there is more front end logic, having it "pop" from grey to another color on a query builder change would be solid / help the user know a parameter changed.

nice catch on the link text within alerts!

* changed button-primary to more sober grey instead of brand-primary
* remove carousel from theme demo page and other useless items
@mistercrunch
Copy link
Member Author

  • changed button-primary to more sober grey instead of brand-primary
  • remove carousel from theme demo page and other useless items

I can't figure out how to get rid of the 404s and how the white link is alterable... Will need help to address those.

@coveralls
Copy link

coveralls commented Jul 22, 2016

Coverage Status

Coverage decreased (-0.01%) to 81.172% when pulling b1c8440fe279bb33b72a25bee8401c0ec52073da on mistercrunch:theme-pasterl into fa0497d on airbnb:master.

@williaster
Copy link
Contributor

I was searching a bit for the link color but no 🎲 yet, I think it must be one of the @...-link-color-... variables.

@mistercrunch
Copy link
Member Author

Alright I cleaned things up (js on main page) and forced a hack for the links to not render in white in Alerts.

@coveralls
Copy link

coveralls commented Jul 26, 2016

Coverage Status

Coverage decreased (-0.02%) to 80.993% when pulling 0f0774c on mistercrunch:theme-pasterl into f43e5f1 on airbnb:master.

@mistercrunch mistercrunch merged commit aaef338 into apache:master Aug 2, 2016
@mistercrunch mistercrunch deleted the theme-pasterl branch August 2, 2016 06:09
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.11.0 labels Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants