Skip to content

Conversation

@CSilivestru
Copy link
Contributor

@CSilivestru CSilivestru commented Apr 17, 2020

Currently displays white on white when no colour theme is provided to the the top bar. The defaults provided didn't seem correct so I'm updating them :).

WHY are these changes introduced?

Addresses issue https://github.com/Shopify/destinations/issues/760.

WHAT is this pull request doing?

Before:
image

After:
image

image

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import React from 'react';
import {Page} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </Page>
  );
}

🎩 checklist

  • Tested on mobile
  • Tested on multiple browsers
  • ~~[ ] Tested for accessibility~~N/A
  • Updated the component's README.md with documentation changes
  • [ ] Tophatted documentation changes in the style guide
  • For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

Chris Silivestru added 2 commits April 17, 2020 11:23
@CSilivestru CSilivestru self-assigned this Apr 17, 2020
@ghost
Copy link

ghost commented Apr 17, 2020

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 17, 2020

🟢 This pull request modifies 4 files and might impact 3 other files.

Details:
All files potentially affected (total: 3)
📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

📄 src/components/TopBar/README.md (total: 0)

Files potentially affected (total: 0)

🎨 src/components/TopBar/components/Menu/Menu.scss (total: 3)

Files potentially affected (total: 3)

🧩 src/utilities/custom-properties/custom-properties.ts (total: 0)

Files potentially affected (total: 0)

@CSilivestru CSilivestru requested a review from alex-page April 17, 2020 17:28
@alex-page alex-page changed the title Update the background menu color when no theme is set. [WIP] Update the background menu color when no theme is set. Apr 17, 2020
@CSilivestru
Copy link
Contributor Author

I made a change to include a var within a var to have polaris attempt to use the background lighter theme override if provided.

The reason this didn't work in earlier trials is because the css variable fallback is treated as plain text and doesn't invoke any functions -- see here.

Because of this, I had to remove the hover variable and place it directly in as the fallback. I'm more than happy to discuss alternatives to this!

@CSilivestru CSilivestru force-pushed the update.hover.and.focus.for.user.menu branch from 44956c4 to 3ad3720 Compare April 20, 2020 17:55
onChange={handleSearchChange}
value={searchValue}
placeholder="Search"
showFocusBorder
Copy link
Member

Choose a reason for hiding this comment

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

To make it easier for the plus team, I updated the example to use their white theme.

Comment on lines -7 to -9
focus-background-color: rgba(color('white'), 0.16),
hover-background-color: rgba(color('white'), 0.08),
active-background-color: rgba(color('black'), 0.28),
Copy link
Member

Choose a reason for hiding this comment

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

These values are used once and blocked multiple theme overrides.

@alex-page alex-page requested a review from alexblaise April 21, 2020 14:54
@alex-page
Copy link
Member

alex-page commented Apr 21, 2020

@alexblaise you should be able to TopHat this when the tests pass there will be a Chroma check that will have a link to an example. I have updated our Themed TopBar Keys example to look like plus global navigation. Let me know if there are any issues.

Copy link

@alexblaise alexblaise left a comment

Choose a reason for hiding this comment

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

Looks great!

@CSilivestru CSilivestru changed the title [WIP] Update the background menu color when no theme is set. Update the background menu color when no theme is set. Apr 21, 2020
Alex Page added 2 commits April 21, 2020 13:59
@alex-page alex-page merged commit d9b19cc into master Apr 21, 2020
@alex-page alex-page deleted the update.hover.and.focus.for.user.menu branch April 21, 2020 20:02
@ghost
Copy link

ghost commented Apr 21, 2020

🎉 Thanks for your contribution to Polaris React!

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.

3 participants