-
Notifications
You must be signed in to change notification settings - Fork 293
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
Fixes #36270 - Update UI to reflect needs_publish on CV #10547
Conversation
Issues: #36270 |
I'm not seeing |
One thought -- why is the "update available" icon on the version rather than somewhere at the top of the CV? If it will only ever be on the latest content view version, I wonder if the upgrade icon should be by the CV name instead. |
That makes sense..Let me run that by Maria and see if we can update that. 👍🏼 |
I think this may only be happening when deleting filters or filter rules rather than creating them. |
Ya..We have some pages on the UI where for responsiveness we do not refresh CV details upon some operations. I guess it's time to change that and fetch CV details on those operations. Let me check where things will need updating. 👍🏼 |
@ianballou : This is ready for re-review with the UI updates on needs_publish changes without refresh. |
Looks like after a publish I'm still seeing the |
Updated to clear needs_publish on UI on publish. 👍🏼 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some small comments, works well otherwise!
} | ||
style={{ marginBottom: '24px' }} | ||
> | ||
<TextContent>{__('Newly published version will be the same as previous version.')}</TextContent> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<TextContent>{__('Newly published version will be the same as previous version.')}</TextContent> | |
<TextContent>{__('Newly published version will be the same as the previous version.')}</TextContent> |
enableFlip | ||
entryDelay={400} | ||
content={composite ? __('Available content view version updates.') : | ||
__('Available repositories and/or filters updates.')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__('Available repositories and/or filters updates.')} | |
__('Publish update available: repositories and/or filters have changed.')} |
what do you think about something like this instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. 👍🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is working for me now!
Merging! 🎉 |
What are the changes introduced in this pull request?
Update CV UI to reflect needs_publish flag and add icons to versions and notice to publish wizard.
Considerations taken when implementing this change?
We have many pages where we avoid reloading the store from API for CV to avoid a refresh of the page. On such components, added a local store variable to reflect needs_publish based on UI actions.
Caveat: Repository level updates like content changes will need a refresh as there's no way to track that on the UI alone.
What are the testing steps for this pull request?
Play around with CV versions and check if the needs_publish icon shows up against the latest version. Also test the publish wizard to see if notice explaining that a publish is not needed shows up when needs_publish is false.