-
Notifications
You must be signed in to change notification settings - Fork 114
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/add deprecation et #2248
Feat/add deprecation et #2248
Conversation
Signed-off-by: Huong Nguyen <huong.nguyen@mckinsey.com>
Signed-off-by: Huong Nguyen <huong.nguyen@mckinsey.com>
Signed-off-by: Huong Nguyen <huong.nguyen@mckinsey.com>
Signed-off-by: Huong Nguyen <huong.nguyen@mckinsey.com>
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.
Gitpod didnt work for me but the screen recording looks good!
Signed-off-by: Huong Nguyen <huong.nguyen@mckinsey.com>
Signed-off-by: rashidakanchwala <rashida_kanchwala@mckinsey.com>
Signed-off-by: rashidakanchwala <rashida_kanchwala@mckinsey.com>
Signed-off-by: rashidakanchwala <rashida_kanchwala@mckinsey.com>
Signed-off-by: rashidakanchwala <rashida_kanchwala@mckinsey.com>
Signed-off-by: rashidakanchwala <rashida_kanchwala@mckinsey.com>
Signed-off-by: rashidakanchwala <rashida_kanchwala@mckinsey.com>
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.
I'm having issues again with my local development environment so can't properly review this atm, sorry.
My impression is that I'm not the only one affected though, seeing the same error on our CI https://github.com/kedro-org/kedro-viz/actions/runs/13054544603/job/36422260077?pr=2255 @rashidakanchwala any clue?
(Pushed an empty commit to trigger the CI) |
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.
Looks great!
Some tiny nits on language :)
// beforeEach(() => { | ||
// // Clears localStorage before each test | ||
// cy.clearLocalStorage(); | ||
// }); |
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.
Delete this?
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.
so i kept it because I will probably uncomment it and add it back. This PR is actually a temp PR, we will undo all the changes next release. Because we won't need deprecation banner anymore
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.
let me remove it anyway. coz in another place, i removed it.
'https://docs.kedro.org/en/stable/integrations/mlflow.html', | ||
'how to continue using Kedro with MLflow for experiment tracking' | ||
)} | ||
, and{' '} |
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.
, and{' '} | |
and{' '} |
Co-authored-by: Tynan DeBold <thdebold@gmail.com> Signed-off-by: rashidakanchwala <37628668+rashidakanchwala@users.noreply.github.com>
}; | ||
|
||
const handleProvideFeedbackClick = () => { | ||
window.open('https://github.com/kedro-org/kedro-viz/issues/2247', '_blank'); |
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.
[Nit] If we want this DeprecationBanner to be a generic one in future, may be we get these issue links as props to the banner. For now may be we can go with this.
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.
hopefully we wont deprecate more features, so this will be a one off thing
return ( | ||
<Modal | ||
className="deprecation-banner-modal" | ||
title="Experiment tracking will be disabled soon." |
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.
[Nit] same as above on the generic deprecation component. I am not sure if the modal design change for each deprecation banner ? But it is worth making a generic banner to extend in future.
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.
Works well 💯 ... Thank you @Huongg @stephkaiser @rashidakanchwala
Description
Fixes #2238, to add a deprecation warning banner to the Kedro-Viz experiment tracking page
Development notes
QA notes
Whether you test locally or via gitpod, the first time it should aways the show warning popup, once the user clicks "acknowledge and dismiss", the warning popup will go away and never be shown again, unless user clears their local storage or uses on a different browser.
et.warning.mov
Checklist
RELEASE.md
file