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

Add new deploy app command #1

Merged
merged 20 commits into from
Sep 30, 2024
Merged

Add new deploy app command #1

merged 20 commits into from
Sep 30, 2024

Conversation

nenb
Copy link
Collaborator

@nenb nenb commented Sep 24, 2024

This PR adds a new command: deploy-app.

deploy-app

  • Opens the URL for the create-app page in a new browser tab. Also passes a query parameter for filepath that is generated from the current workspace in JLab. Accepts an argument origin ('menu', 'context-menu' etc) that allows controlling what icon is displayed.

Reference Issues or PRs

What does this implement/fix?

Put a x in the boxes that apply

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds a feature)
  • Breaking change (fix or feature that would cause existing features not to work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • Other (please describe):

Testing

  • Did you test the pull request locally?
  • Did you add new tests?

Documentation

Access-centered content checklist

Text styling

  • The content is written with plain language (where relevant).
  • If there are headers, they use the proper header tags (with only one level-one header: H1 or # in markdown).
  • All links describe where they link to (for example, check the Nebari website).
  • This content adheres to the Nebari style guides.

Non-text content

  • All content is represented as text (for example, images need alt text, and videos need captions or descriptive transcripts).
  • If there are emojis, there are not more than three in a row.
  • Don't use flashing GIFs or videos.
  • If the content were to be read as plain text, it still makes sense, and no information is missing.

Any other comments?

Original issue: nebari-dev/jhub-apps#472

Copy link
Contributor

Binder 👈 Launch a Binder on branch nebari-dev/jupyterlab-jhub-apps/create-apps-new-window

@nenb nenb added needs: review 👀 This PR is complete and ready for reviewing feature labels Sep 24, 2024
Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

Thank you, amazing! You should be able to manually update the snapshots by downloading the files from https://github.com/nebari-dev/jupyterlab-jhub-apps/actions/runs/11022641935?pr=1

image

Let's see if bot works here:

bot please update snapshots

junit.xml Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
style/icons/deploy-app.svg Outdated Show resolved Hide resolved
ui-tests/tests/jupyterlab_jhub_apps.spec.ts Outdated Show resolved Hide resolved
@krassowski
Copy link
Member

bot please update snapshots

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this did not work. Maybe needs a timeout?

Looking at the darwin snapshot:

  • the kernel busy indicator (the black circle) will flip on and off depending on the kernel state; we may want to ensure that kernel is in idle state (have a look at the tests in JupyterLab to see how to do that)
  • the deploy icon looks great here; I think the mockups also included a "Deploy App" label when displayed in the toolbar, do you want to include it straight away or in a follow-up PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will add the "Deploy App" label in the toolbar in a follow-up PR, just to try and get this PR merged.

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@nenb
Copy link
Collaborator Author

nenb commented Sep 26, 2024

bot please update snapshots

@krassowski
Copy link
Member

Updating snapshots failed because of timeouts I think:

https://github.com/nebari-dev/jupyterlab-jhub-apps/actions/runs/11060426290/job/30730934981

Screenshot from 2024-09-27 10-20-38

@nenb
Copy link
Collaborator Author

nenb commented Sep 27, 2024

bot please update snapshots

@nenb
Copy link
Collaborator Author

nenb commented Sep 27, 2024

bot please update snapshots

Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

Thank you @nenb!

For context, this PR was previously reviewed in nebari-dev/jupyterlab-nebari-mode#7.

@krassowski krassowski merged commit 0e0faf6 into main Sep 30, 2024
6 checks passed
@krassowski krassowski deleted the create-apps-new-window branch September 30, 2024 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature needs: review 👀 This PR is complete and ready for reviewing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants