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: Add Prop to NonIdealState to Disable Muted Icon Style #6611

Merged
merged 11 commits into from
Jan 4, 2024

Conversation

AlexKempen
Copy link
Contributor

Introduction

This PR adds a prop mutedIcon to NonIdealState which can be used to explicitly opt-out of the muted style for the icon. This makes it possible to, for example, use an <Icon> component with an intent specified alongside NonIdealState, which is currently impossible since Classes.NON_IDEAL_STATE_VISUAL always overrides the intent (even when passing an explicit <Icon> JSX element).

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

This PR adds a mutedIcon prop to NonIdealState. mutedIcon defaults to true to preserve backwards-comparability. When set to false, Classes.NON_IDEAL_STATE_VISUAL is no longer applied to the icon prop, which can prevent issues with certain props getting overridden in an undesirable (and unexpected) way.

Screenshot

Example of a NonIdealState with a custom <Icon> and intent:
fix

First Time Contributor Notes

As a first time contributor on this repo, I'll briefly discuss my experience getting the code to run and build. I first tried on my Windows 10 machine, which I ultimately gave up on. The primary issues I encountered there were:

  • windows-build-tools refused to install for me. The primary suggestion I saw while troubleshooting was that it is now natively available with Node as an option during install. I'm not sure if that worked for me or not.
  • Issues with C: paths being disallowed. The suggestion I saw while troubleshooting was to use a later version of Node (the post said they saw it resolved going from Node 12 to Node 16), but it was nevertheless occurring with the project's Node 18.

Since I couldn't find any fixes for the above issues, I eventually gave up and switched to Ubuntu in WSL, which worked much better. My main feedback there is that ChromeHeadless is needed to run yarn test, which isn't mentioned in the documentation (as far as I can tell). These commands resolved it for me:

wget https://dl.google.com/linux/direct/google-chrome-stable_current_amd64.deb
sudo apt install ./google-chrome-stable_current_amd64.deb

@palantirtech
Copy link
Member

Thanks for your interest in palantir/blueprint, @AlexKempen! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and your notes about the contribution process @AlexKempen. I'm aware of development issues in Windows environments, we haven't had the bandwidth to fix them but we're open to PRs (see #6241)

As for the contents of this PR: I'd suggest moving the muted styling to an "icon modifier" class - see comments below. That would mean that _non-ideal-state.scss changes to:

.#{$ns}-non-ideal-state-visual {
  color: $gray3;
}

and the removed styles move to _icon.scss:

#{$ns}-icon {
  // ... existing styles

  &.#{$ns}-icon-muted svg {
    fill-opacity: 15%;
    // need to show overflow for some strokes on paths that reach the edge of the icon bounding box
    overflow: visible;

    path {
      stroke: $gray3;
      stroke-opacity: 50%;
      stroke-width: 0.5px;
    }
  }

  .#{$ns}-dark & {
    .#{$ns}-icon-muted svg {
      fill-opacity: 20%;
    }
  }
}

and we get a new string constant for the modifier class in classes.ts:

export const ICON_MUTED = `${ICON}-muted`;

@adidahiya adidahiya self-assigned this Dec 20, 2023
@AlexKempen
Copy link
Contributor Author

@adidahiya ping since it's been a little while - is there anything else you'd like me to do with this?

Copy link
Contributor

@adidahiya adidahiya 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, thanks for making all those adjustments @AlexKempen!

the new prop looks good in docs preview, and I was able to toggle it to true using React dev tools:

image

@adidahiya adidahiya merged commit b90ef52 into palantir:develop Jan 4, 2024
12 checks passed
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.

None yet

3 participants