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

fix: remove compass-preferences from connection-form completely COMPASS-8098 #6294

Merged
merged 2 commits into from
Sep 30, 2024

Conversation

paula-stacho
Copy link
Contributor

Description

Removing last reference to compass-preferences from connection-form, because it's causing some issues in the VSCode integration.

Checklist

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@github-actions github-actions bot added the fix label Sep 27, 2024
Copy link
Member

@Anemy Anemy left a comment

Choose a reason for hiding this comment

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

lgtm! One suggestion on some old code we can remove, not a blocker, feel free to merge as is

connectionColorToHex: isMultiConnectionEnabled
? newColorCodeToHex
: colorCodeToHex,
connectionColorToHex: newColorCodeToHex,
Copy link
Member

Choose a reason for hiding this comment

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

[nit] Does it make sense to drop the new now?

return;
}

const migratedColor = legacyColorsToColorCode(colorCode);
Copy link
Member

Choose a reason for hiding this comment

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

We only have one other use of legacyColorsToColorCode, and it's in the color picker. Maybe we can remove legacyColorsToColorCode and LEGACY_COLORS_TO_COLOR_CODE_MAP now? Someone would be using a color from ~3 years ago, and we'd only slightly be helping them select a color inside of the color picker UI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that must be part of the legacy bits. I'll try to remove it + the parts that use it, hopefully it's not too deep of a rabbit hole 🤞

@paula-stacho paula-stacho added the no release notes Fix or feature not for release notes label Sep 30, 2024
@paula-stacho paula-stacho merged commit be3d5f9 into main Sep 30, 2024
27 of 28 checks passed
@paula-stacho paula-stacho deleted the COMPASS-8089-2 branch September 30, 2024 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix no release notes Fix or feature not for release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants