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

Using update_only when doing background discover and content change #955

Merged
merged 19 commits into from
Feb 1, 2024

Conversation

travjenkins
Copy link
Member

@travjenkins travjenkins commented Jan 31, 2024

Issues

#953
#954

Changes

953

  • Updated content

954

  • Set the update_only column based on if we are rediscovering
  • Allow passing in settings for update_only when discovering

Misc

  • Resetting draft id when discover fails to prevent incorrect test/saves

Tests

Manually tested

  • 953: viewed edit and create for capture

Automated tests

  • N/A

Screenshots

Edit content updated
image

Create Content not changed
image

Clicking the refresh button still wants to do a discover so the user can get new items
image

@travjenkins travjenkins changed the title Updating content to make it more clear to users they are editing exis… Using update_only when doing background discover and content change Jan 31, 2024
@travjenkins travjenkins marked this pull request as ready for review January 31, 2024 03:22
@travjenkins travjenkins requested a review from a team as a code owner January 31, 2024 03:22
@travjenkins travjenkins merged commit 54439e7 into main Feb 1, 2024
3 checks passed
@travjenkins travjenkins deleted the travjenkins/uxTweaks4 branch February 1, 2024 14:55
const { generateCatalog, isSaving, formActive } = useDiscoverCapture(
entityType,
{
// We only want to set updateOnly if the user is editing and not updating the config
Copy link
Member

Choose a reason for hiding this comment

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

and not updating the config

Given the presence of the useResourceConfig_rediscoveryRequired hook above, it should be clear that the resource config is what is being referred to by the term config. However, it could not hurt to explicitly state that updateOnly should be set if the user (clicked the Next CTA and) did not change the resource config in an edit workflow.

const { generateCatalog, isSaving, formActive } = useDiscoverCapture(
entityType,
{
// We only want to set updateOnly if the user is editing and not updating the config
// This should cover when a user has enable previously disabled collection(s)
Copy link
Member

Choose a reason for hiding this comment

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

a user has enable previously disabled collection(s)

Removing the passive voice from this comment will increase its clarity: This should cover when a user enables previously disabled collection(s).

if (discoverResponse.error) {
// If we failed at discovery we need to clear draft ID like we do
Copy link
Member

Choose a reason for hiding this comment

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

This comment is a little muddled to me. What do you think of the following:

// If discovery fails, clear the draft ID to preserve the _Next_ CTA and allow the user to attempt to discover another time.

@@ -21,7 +21,7 @@ function MessageCell({ fields, message }: Props) {
</Typography>

{fields ? (
<Typography component="div" sx={BaseTypographySx}>
<Typography component="div" sx={{ ...BaseTypographySx }}>
Copy link
Member

Choose a reason for hiding this comment

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

The spread operator should not be needed here.

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