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

Improve active icon states #915

Merged
merged 11 commits into from
Jun 16, 2022
Merged

Improve active icon states #915

merged 11 commits into from
Jun 16, 2022

Conversation

cvetankanechevska
Copy link
Contributor

@cvetankanechevska cvetankanechevska commented Jun 15, 2022

Description

Resolves #902

Development notes

Added active status for icons that represent a "toggle like action":

  1. Flowchart - toolbar: MenuIcon, Minimap, LabelIcon and LayersIcon
  2. Experiment tracking: Bookmark, ShowChangesIcon, KebabIcon and PinIcon

QA notes

Tested locally on dark and light theme.

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

@tynandebold tynandebold requested a review from comym June 16, 2022 09:36
Copy link
Member

@tynandebold tynandebold left a comment

Choose a reason for hiding this comment

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

This looks great. I'm approving straight away with just one small comment about a coding style I try to follow within the project.

@@ -42,6 +42,7 @@ export const ExperimentPrimaryToolbar = ({
labelText={`${selectedRunData?.bookmark ? 'Unbookmark' : 'Bookmark'}`}
onClick={() => toggleBookmark()}
visible={!enableComparisonView}
active={selectedRunData?.bookmark}
Copy link
Member

Choose a reason for hiding this comment

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

Nit (coding style): I try to advise us to alphabetically sort component props, and also when destructuring and even CSS styles, where applicable. It makes readability easier when you know there's an order to things. There's a great VSCode extension that makes this easy, and would assume there's ways to do this in other IDEs as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I will update it.

Copy link
Contributor

@Huongg Huongg left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@tynandebold tynandebold merged commit a9e17b4 into main Jun 16, 2022
@tynandebold tynandebold deleted the improve-active-icon-states branch June 16, 2022 15:45
@tynandebold tynandebold mentioned this pull request Jun 16, 2022
5 tasks
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.

Improve active icon states in utility bar
4 participants