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

chore(aggregations): Refactor aggregations stage toolbar #6572

Merged
merged 2 commits into from
Dec 18, 2024

Conversation

kraenhansen
Copy link
Contributor

@kraenhansen kraenhansen commented Dec 17, 2024

Description

An alternative to the refactor which happened in #6569 (comment) favoring composition over arrays of props.

I included a change to re-export the GlyphName from LG instead of deriving and exporting our own from the "components" package.

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)

@kraenhansen kraenhansen requested a review from mabaasit December 17, 2024 13:36
@kraenhansen kraenhansen self-assigned this Dec 17, 2024
@kraenhansen kraenhansen marked this pull request as draft December 17, 2024 14:08
@kraenhansen kraenhansen marked this pull request as ready for review December 17, 2024 14:08
Comment on lines +53 to +54
onClick={() => onAddStageClick(index)}
setMenuOpen={setMenuOpen}
Copy link
Contributor

Choose a reason for hiding this comment

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

What benefit does it have to pass these two here instead of just onClick and then something like:

	onClick={() => {
        onAddStageClick(index);
        setMenuOpen(false);
    }}

Copy link
Contributor

Choose a reason for hiding this comment

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

not a blocker though.

Copy link
Contributor Author

@kraenhansen kraenhansen Dec 17, 2024

Choose a reason for hiding this comment

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

This just ensures that setMenuOpen(false) is always called when onClick is called on any item.. Not making that a choice of the caller.

@kraenhansen kraenhansen added the no-title-validation Skips validation of PR titles (conventional commit adherence + JIRA ticket inclusion) label Dec 17, 2024
@kraenhansen kraenhansen merged commit 7a89701 into main Dec 18, 2024
32 of 35 checks passed
@kraenhansen kraenhansen deleted the kh/refactor-aggregations-stage-toolbar branch December 18, 2024 05:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-title-validation Skips validation of PR titles (conventional commit adherence + JIRA ticket inclusion)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants