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

[D&D] Adds count field to field picker #2231

Merged
merged 1 commit into from
Aug 31, 2022

Conversation

ashwin-pc
Copy link
Member

Signed-off-by: Ashwin Pc ashwinpc@amazon.com

Description

Adds a Count field to the field picker in Wizard

Screen Shot 2022-08-31 at 3 15 15 AM

Issues Resolved

Check List

  • New functionality includes testing.
    • All tests pass
      • yarn test:jest
      • yarn test:jest_integration
      • yarn test:ftr
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

Signed-off-by: Ashwin Pc <ashwinpc@amazon.com>
@ashwin-pc ashwin-pc requested a review from a team as a code owner August 31, 2022 10:15
@ashwin-pc ashwin-pc self-assigned this Aug 31, 2022
@ashwin-pc ashwin-pc requested a review from CPTNB August 31, 2022 10:15
Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

I like that most of the special case handling is confined to the use_dropbox hook, and I think this is a good start for now. But it reads as one-off handling of a special case. That may actually be the correct model for how to handle it, but it just makes me wonder if some of the other generalized assumptions about fields (and even some of the naming) is a bit too narrow to comfortably accommodate this more primitive field. No additional suggestions, just something we'll want to keep an eye on if we find ourselves adding more exceptions in the future.


export interface EmptyDragDataType {
namespace: null;
value: null;
}
export interface FieldDragDataType {
namespace: 'field-data';
value: Pick<IndexPatternField, 'name' | 'displayName' | 'type'> | null;
value: IndexPatternField['name'] | null | typeof COUNT_FIELD;
Copy link
Member

Choose a reason for hiding this comment

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

nit - I can't particularly justify why, but I think it reads nicer if null is last in the union.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will update it in a follow-up PR

@joshuarrrr
Copy link
Member

One other question - because count is applicable to any index pattern, there's no reason to clear it when switching index patterns. Should we create another issue/PR to make sure it persists in that case?

Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

Nice!

@ashwin-pc
Copy link
Member Author

@joshuarrrr I agree. I was tempted to add another prop to identify the count field since there are many ways to slice and dice this. But I went with this approach since it was cleaner and didn't need a lot of overhead. If we need to add any more exceptions, I will refactor to be more generic in the way we handle them.

As for count applying to any index pattern, it's not clear which schemas support it though. So to persist it in a new schema will need a lil thought. I want to work on persisting aggs when vis types change as a separate problem and solve for it in a generic way

@ashwin-pc ashwin-pc merged commit aaa35c3 into opensearch-project:main Aug 31, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 31, 2022
Signed-off-by: Ashwin Pc <ashwinpc@amazon.com>

Signed-off-by: Ashwin Pc <ashwinpc@amazon.com>
(cherry picked from commit aaa35c3)
ananzh pushed a commit that referenced this pull request Sep 6, 2022
Signed-off-by: Ashwin Pc <ashwinpc@amazon.com>

Signed-off-by: Ashwin Pc <ashwinpc@amazon.com>
(cherry picked from commit aaa35c3)

Co-authored-by: Ashwin P Chandran <ashwinpc@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants