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

feat(compass-global-writes): sharding error state COMPASS-8349 #6374

Merged
merged 12 commits into from
Oct 29, 2024

Conversation

paula-stacho
Copy link
Contributor

@paula-stacho paula-stacho commented Oct 17, 2024

Description

Notes:

I have thought about whether this state needs to be separate, or if it would better be the unsharded state with optional error. In the end I continued the approach with separate states and the store behaving as a state machine. It still makes sense to me and I find it better than having a ton of conditional bits in the components. But, I've been working on this for a while, so I'm definitely biased and I wouldn't be surprised if the solution isn't that clear to someone else - let me know if that's the case and we can talk about how to make it clearer!

Preview:
if you happen to read the error message - this one actually should be handled in a different way (key mismatch screen), but this was the easiest for me to reproduce
Screenshot 2024-10-24 at 14 40 09

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)

@paula-stacho paula-stacho changed the title feat(compass-global-writes): sharding error state feat(compass-global-writes): sharding error state COMPASS-8349 Oct 17, 2024
@github-actions github-actions bot added the feat label Oct 17, 2024
onClick={onSubmit}
disabled={!secondShardKey || isSubmittingForSharding}
variant="primary"
isLoading={isSubmittingForSharding}
Copy link
Contributor Author

@paula-stacho paula-stacho Oct 17, 2024

Choose a reason for hiding this comment

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

Small change here. It used to be:

disabled={!secondShardKey || isSubmittingForSharding}
variant="primary"
leftGlyph={
  isSubmittingForSharding ? (
    <SpinLoader title="Creating shard key" />
  ) : undefined
}

{
onCreateShardKey: createShardKey,
}
)(CreateShardKeyForm);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another small change - this is now connected directly, and it works with error/no error statuses.

@paula-stacho paula-stacho force-pushed the COMPASS-8349-separate-status branch 2 times, most recently from c203243 to aa0be2d Compare October 23, 2024 13:59
@paula-stacho paula-stacho force-pushed the COMPASS-8349-separate-status branch from aa0be2d to 0b19805 Compare October 24, 2024 09:07
@paula-stacho paula-stacho added the no release notes Fix or feature not for release notes label Oct 24, 2024
@paula-stacho paula-stacho marked this pull request as ready for review October 24, 2024 11:34
import { css, spacing } from '@mongodb-js/compass-components';

export const bannerStyles = css({
textAlign: 'justify',
Copy link
Contributor

Choose a reason for hiding this comment

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

this adds extra space in the banner and the message looks a bit weird (in the screenshot attached in the PR description). also, would be nice if we can split the error message by new line (server error has it).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed this setting for the error specifically. otherwise most messages I tried looked better with justify. also added pre-wrap on the error.

@paula-stacho paula-stacho force-pushed the COMPASS-8349-separate-status branch from d6cf33e to 9d194dd Compare October 28, 2024 18:34
@paula-stacho paula-stacho force-pushed the COMPASS-8349-separate-status branch from 9d194dd to 950e100 Compare October 29, 2024 11:46
@paula-stacho paula-stacho merged commit b377452 into main Oct 29, 2024
29 checks passed
@paula-stacho paula-stacho deleted the COMPASS-8349-separate-status branch October 29, 2024 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat 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