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

[Jupyter] Basic PPS UI #8557

Merged
merged 6 commits into from Mar 7, 2023
Merged

[Jupyter] Basic PPS UI #8557

merged 6 commits into from Mar 7, 2023

Conversation

bbonenfant
Copy link
Contributor

@bbonenfant bbonenfant commented Feb 1, 2023

This is a very simple (first draft) of the UI for the PPS plugin for Jupyterlab. This doesn't include any backend calls. This my first PR for front end work so I'm sure a lot of it is incorrect, please tear it apart!

Question for @smalyala : I'm not sure that we want this merged into master yet (I don't know how the plugin releases are currently being done). Do you have thoughts about where this line of work should live?

Jira: INT-665

@smalyala
Copy link
Contributor

Let's start a feature branch for the pipeline extension, which we merge into master once the pipeline extension is ready.

@@ -203,7 +217,22 @@ export class MountPlugin implements IMountPlugin {
);
this._datum.addClass('pachyderm-mount-datum-wrapper');

this._pipeline = ReactWidget.create(
Copy link
Contributor

@smalyala smalyala Feb 13, 2023

Choose a reason for hiding this comment

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

We touched upon this during the FE walkthrough with @pleeko in Miami, but we probably don't need to use signals here since the pipeline component doesn't have any properties/data that changes like the other components does. Signals are useful in the case where we see that some data has changed so we re-render the components that depend on that data. But since the pipeline component doesn't take any input that could change, we can remove signals.

Now, how this would look with no signals, I defer to Peter. It might actually just be removing the signal code lines that wrap the component and removing lines of code that emit to the corresponding signal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the signal in a way that made sense to me, but I'll wait for Peter's feedback

Copy link
Member

@pleeko pleeko left a comment

Choose a reason for hiding this comment

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

LGTM

@bbonenfant bbonenfant merged commit 0aa8e8b into master Mar 7, 2023
@bbonenfant bbonenfant deleted the bonenfant/jupyter-pipeline-ui branch March 7, 2023 18:47
bbonenfant added a commit that referenced this pull request Mar 9, 2023
MVP UI for PPS extension. Nothing hooked up to the backend.
bbonenfant added a commit that referenced this pull request Mar 14, 2023
MVP UI for PPS extension. Nothing hooked up to the backend.
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.

None yet

3 participants