Skip to content

Conversation

mabaasit
Copy link
Collaborator

@mabaasit mabaasit commented Feb 15, 2022

feat(compass-saved-aggregations-queries): update query/aggregation COMPASS-5506

Description

Checklist

  • New tests and/or benchmarks are included
  • Documentation is changed or added

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)

@mabaasit mabaasit marked this pull request as draft February 15, 2022 21:06
Copy link
Collaborator

@gribnoysup gribnoysup left a comment

Choose a reason for hiding this comment

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

Overall I think it looks pretty good, there are a bunch of things that caught my eye, but let me know if these don't make too much sense to you

onCancel: () => void;
};

const formTitleStyles = css({
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use this code as a reference to how leafygreen styles this component

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks. made it close to this, except for margin-bottom. Sticking to a number provided by spacing that fits.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I should've been more clear, I meant like all the modal styles in this file I linked to: we are missing the divider between controls and confirm etc., but it's fine like that too, I have a ticket for creating a modal form component for compass-components, so we can pick it up later (there are a few other places in compass where it needs to be used anyway)

@mabaasit mabaasit marked this pull request as ready for review February 22, 2022 16:06
@mabaasit mabaasit requested a review from gribnoysup February 22, 2022 16:40
Copy link
Collaborator

@gribnoysup gribnoysup left a comment

Choose a reason for hiding this comment

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

Looks good, a few small comments, but feel free to ignore

onCancel: () => void;
};

const formTitleStyles = css({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I should've been more clear, I meant like all the modal styles in this file I linked to: we are missing the divider between controls and confirm etc., but it's fine like that too, I have a ticket for creating a modal form component for compass-components, so we can pick it up later (there are a few other places in compass where it needs to be used anyway)

import { formatDate } from '../utlis/format-date';

export type Action = 'open' | 'delete' | 'copy' | 'rename';
export type Action = 'open' | 'delete' | 'copy' | 'edit';
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd double-check the name with product just in case. IIRC we wanted it to be "Rename" because "Edit" reads as if you can edit the whole thing, but you can only rename it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed it to rename.

: mapAggregationToItem(action.payload as Aggregation);
return {
...state,
items: [...state.items.filter((x) => x.id !== action.id), updatedItem],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Total nit, but can be done in one go (instead two iterations)

Suggested change
items: [...state.items.filter((x) => x.id !== action.id), updatedItem],
items: state.items.map((item) => {
if (item.id === action.id) {
return updatedItem;
} else {
return item
}
}),

@mabaasit mabaasit merged commit efd5e7a into main Feb 24, 2022
@mabaasit mabaasit deleted the COMPASS-5506-update-item branch February 24, 2022 09:14
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.

2 participants