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 dashboard table view #6015

Merged
merged 260 commits into from
Apr 13, 2023
Merged

Conversation

somebody1234
Copy link
Collaborator

@somebody1234 somebody1234 commented Mar 20, 2023

Pull Request Description

  • Adds dummy views for all asset types (project, directory, secret, file)
    • The backend metadata does not exist for many of these columns.

Important Notes

To do:

  • figure out the best way to add the custom classes - tailwind likes to avoid custom classes whenever possible

Screenshots

image

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@somebody1234
Copy link
Collaborator Author

for posterity (since new commits have changed line numbers):
lib/content/src/index.ts:148 is

if (auth) auth.style.display = 'none'

lib/content/src/index.ts:169 is the last line here:

if (!appInstanceRan) {
appInstanceRan = true
void appInstance.run()

@@ -150,4 +158,4 @@
}
}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

no nl

@@ -40,7 +40,7 @@
<script type="module" src="/run.js" defer></script>
</head>
<body>
<div id="dashboard"></div>
<div id="dashboard" class="dashboard"></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

can we change id and class to enso-dashboard? then we probably need also update prefix in tailwindcss

@@ -13,13 +13,9 @@ const ESBUILD_PATH = '/esbuild'
const ESBUILD_EVENT_NAME = 'change'

// ===================
// === Live reload ===
// === Live relaod ===
Copy link
Contributor

Choose a reason for hiding this comment

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

reload

@PabloBuchu
Copy link
Contributor

Thanks for the flags. Please address comments and open for review

@somebody1234
Copy link
Collaborator Author

@PabloBuchu can't repro, sorry - logs show that the project manager is being started:

│ Loading the window address 'http://localhost:8080?startup.platform=linux&cloud.authentication=true&engine.preferredVersion=2023.1.1-nightly.2023.4.11'.
╰ Done in 2481.7 ms.
[info] [2023-04-12T04:46:01.6Z] [org.enso.projectmanager.boot.ProjectManager$] Starting Project Manager...
[info] [2023-04-12T04:46:02.032Z] [org.enso.projectmanager.boot.ProjectManager$] Started server at 127.0.0.1:30535, press enter to kill server

(did a fresh rebuild just to double-check)
that said, this is using the new flags instead of commenting likes out, that might affect things

@somebody1234 somebody1234 marked this pull request as ready for review April 12, 2023 05:03
@PabloBuchu
Copy link
Contributor

@somebody1234 so one minor issue: -cloud.authentication=true doesn't show login templates without -cloud.dashboard=true

listing projects from project_manager works fine, but clicking on open project calls for API. We could leave it for now and fix it in #5804 this PR

@somebody1234
Copy link
Collaborator Author

@PabloBuchu login templates?
if you mean the dashboard UI, that's intentional - otherwise i'm not quite sure what having a separate dashboard option is supposed to do
(right now, there is code to explicitly only show the dashboard if cloud.dashboard=true

@PabloBuchu
Copy link
Contributor

@PabloBuchu login templates? if you mean the dashboard UI, that's intentional - otherwise i'm not quite sure what having a separate dashboard option is supposed to do (right now, there is code to explicitly only show the dashboard if cloud.dashboard=true

@somebody1234
so when we use authentication the user should be welcomed with login page (sign in with github etc) but dashboard flag should indicate whether he lands on old IDE (appInstance.run in onAuthenticated method) or see the new dashboard with new table view introduced by this PR

@somebody1234
Copy link
Collaborator Author

somebody1234 commented Apr 12, 2023

@PabloBuchu a bit late but i pushed a commit that should fix it
(for posterity, it's because DASHBOARD_PATH is /, i.e. the dashboard is the default route - and it navigates away to the authentication route(s) when the user is not logged in. one of the previous commits disabled this route entirely; this one adds it back but makes it blank)

@PabloBuchu
Copy link
Contributor

@somebody1234 did you pushed your latest changes? Because I still can't get to work cloud flags properly

  1. -cloud.authentication=true should allow user for signing in and then redirect to old IDE. Instead I am getting a blank page
Screen.Recording.2023-04-13.at.10.59.26.mov
  1. If I was logged in previously the flag works fine and I see the old IDE

Uploading Screen Recording 2023-04-13 at 11.00.53.mov…

  1. -cloud.authentication=true -cloud.dashboard=true looks like it works fine
Screen.Recording.2023-04-13.at.11.02.21.mov

@somebody1234
Copy link
Collaborator Author

whoops :|
pushed now @PabloBuchu

@PabloBuchu PabloBuchu added the CI: No changelog needed Do not require a changelog entry for this PR. label Apr 13, 2023
@PabloBuchu
Copy link
Contributor

Thanks works now.

QA:

  • flags: 🟢
  • build & run: 🟢
  • ide watch: 🟢
  • gui watch: 🟢

@wdanilo can you please take a look and review the code?

logger: loggerProvider.Logger
platform: platformModule.Platform
/** Whether the dashboard should be rendered. */
enableDashboard: boolean
Copy link
Member

Choose a reason for hiding this comment

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

minor thing, but I think showDashboard would be a better name. enableDashboard makes me confused what "enabling" it really means, while, according to docs, this is only about showing it.

@PabloBuchu PabloBuchu added the CI: Ready to merge This PR is eligible for automatic merge label Apr 13, 2023
@mergify mergify bot merged commit 72ae10c into develop Apr 13, 2023
23 checks passed
@mergify mergify bot deleted the wip/somebody1234/dashboard-directory-view branch April 13, 2023 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants