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

feat: remove focusRedesignOptOut feature flag #7076

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

tomdavies73
Copy link
Contributor

Proposed behaviour

This update removes the focusRedesignOptOut feature flag, consumers can no longer opt out of the dual outline focus styling

Current behaviour

Currently, consumers can opt out of the dual outline focus styling.

Checklist

  • Commits follow our style guide
  • Related issues linked in commit messages if required
  • Screenshots are included in the PR if useful
  • All themes are supported if required
  • Unit tests added or updated if required
  • Playwright automation tests added or updated if required
  • Storybook added or updated if required
  • Translations added or updated (including creating or amending translation keys table in storybook) if required
  • Typescript d.ts file added or updated if required
  • Related docs have been updated if required

QA

  • Tested in provided StackBlitz sandbox/Storybook
  • Add new Playwright test coverage if required
  • Carbon implementation matches Design System/designs
  • UI Tests GitHub check reviewed if required

Additional context

Testing instructions

@tomdavies73 tomdavies73 self-assigned this Nov 14, 2024
@tomdavies73 tomdavies73 requested review from a team as code owners November 14, 2024 14:08
@tomdavies73 tomdavies73 changed the title feat: remove focusRedesignOptOut feature flag **WIP** feat: remove focusRedesignOptOut feature flag Nov 14, 2024
@DipperTheDan DipperTheDan self-requested a review November 15, 2024 13:52
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: You may be able to remove theme from line 156 as it looks like it isn't used elsewhere in the file.

@@ -278,11 +268,7 @@ const StyledDayPicker = styled.div`
${({ theme }) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: It might be the same case here that we can remove the theme here. From a glance, I cannot see it being used, but I may be wrong, so please double-check 😄

@Parsium Parsium self-requested a review November 18, 2024 16:16
Copy link
Contributor

@Parsium Parsium left a comment

Choose a reason for hiding this comment

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

Great work on this @tomdavies73 👍🏼 I noticed a few unused theme variables in some parts of the code. Could you take a look and remove them?

To identity these, this command should help:

npx eslint ./src --rule "{ '@typescript-eslint/no-unused-vars': 'error' }" --quiet

Thanks!

Parsium
Parsium previously approved these changes Nov 19, 2024
DipperTheDan
DipperTheDan previously approved these changes Nov 19, 2024
paulrobinson88
paulrobinson88 previously approved these changes Nov 21, 2024
This update removes the `focusRedesignOptOut` feature flag, consumers can no longer opt out of the
dual outline focus styling

BREAKING CHANGE: The `focusRedesignOptOut` feature flag has been removed completely. If passed to
the `CarbonProvider` it will need to be removed, if consumers are also relying on previous
focus styling this will also no longer be available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants