-
Notifications
You must be signed in to change notification settings - Fork 113
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
[KED-2533] Fix/delete description from metadata #403
Conversation
This was unused reducer in the visible.js leftover from a previous changed. layers.visible reducer is used to store the state of layers.
Deleted description (docstring) from meta_data panel as it already present in the code panel.
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.
LGTM :) Thanks Rashida!
Only thing is that the description field should ideally only be merged in after the code panel flag is removed (https://jira.quantumblack.com/browse/KED-2534).
Happy to approve this once the above change is merged in main
…ntumblacklabs/kedro-viz into fix/delete-description-from-metadata
This reverts commit f85530e.
Code panel is now a default feature hence the flag is not required.
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.
Great work! I didn't realise that we were deleting this feature but sounds good to me.
I searched the codebase and there are still a few leftover mentions of docstring
that need removing:
/src/reducers/nodes.js
on lines 59-61/src/reducers/reducers.test.js
on line 217/src/store/normalize-data.js
on lines 29 & 143/src/utils/random-data.js
on line 200/src/utils/data/node_task.mock.js
on line 4
We should also check whether we should remove it from the Python server (package/kedro_viz/server.py
) - @limdauto @MerelTheisenQB should we remove docstring
from the API node response?
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.
EDIT: Deleted duplicate comment due to server request error.
@MerelTheisenQB - I have removed docstring related test case from test_server.py -- I was not super sure if it was relevant to it. def test_node_metadata_endpoint_task_missing_docstring. Please let me know if it's ok |
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 all good on the python side! 🐍 👍 Nice one @rashidakanchwala 😄
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.
Great work!
Description
The docstring is already present in the code panel hence deleting it from metadata panel
Development notes
The docstring is already present in the code panel hence deleting it from metadata panel
Removed docstring reference from
QA notes
Ran npm t successfully
Checklist
RELEASE.md
fileLegal notice
I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":
I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorised to submit this contribution on behalf of the original creator(s) or their licensees.
I certify that the use of this contribution as authorised by the Apache 2.0 license does not violate the intellectual property rights of anyone else.