-
Notifications
You must be signed in to change notification settings - Fork 1
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
(web) add a console component that allows running sql in the context of an env #293
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
76e9ba7
to
cbb37d5
Compare
cbb37d5
to
4eab6a0
Compare
6c443f6
to
9ec7d4f
Compare
|
||
// splitting on "/" means there is always an empty string as the first | ||
// element in the Array since pathname always starts with / | ||
const isConsole = pathParts.length === 5 && pathParts.pop() === "console"; |
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.
@asutula curious if you have any thoughts what to do here?
The url /joe/foo/default/console
could be a table page or the console page (or really any url with 4 parts). We could refactor our url scheme to allow for unique page urls regardless of name, or add console as a reserved word. Using a reserved word feels wrong to me. If we do that it would mean that anyone with an already existing def called console
would have a broken project.
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 do have a suggestion that is a bit cleaner:
You can rely on the values returned by useParams
above to distinguish between the console
part of the route and any table name. If the user is visiting the console
page, defSlug
will be undefined
. So this give us most of the information we need to make sure we're not viewing a table (def), but we still need to distinguish between the overview page and console page which both match the case !!envSlug && !defSlug
. We can use the hook useSelectedLayoutSegments
to get a nice array of url segments below the layout without needing to split the pathname string on /
etc (as you complain about below). So the final calculation of isConsole
would be:
const isConsole =
!!envSlug && !defSlug && selectedLayoutSegments.slice(-1)[0] === "console";
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.
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.
We can use the hook useSelectedLayoutSegments to get a nice array of url segments below the layout without needing to split the pathname string on / etc (as you complain about below). So the final calculation of
isConsole
would be:const isConsole = !!envSlug && !defSlug && selectedLayoutSegments.slice(-1)[0] === "console";
The useSelectedSegments
hook works well, I'll switch to that.
We should, for now at least, just go with the restricted slug approach. There is no table def named console yet (I checked using your console!), so we can implement that restriction with no problem currently.
I'll set it up so that "console" is a reserved word within the context of defs. A similar problem exists if you create an environment called "settings". should I work on fixing that here, or leave it for another PR?
hightlightWithLineNumbers( | ||
code, | ||
(languages as any).sql, | ||
props.hideLineNumbers, |
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 left the line numbers feature mostly in place, but hard coded it to hide the line numbers. If we want to show line numbers we will need to include the css the old console was using, or convert it to tailwind and add the classes.
Just saw this @joewagner. Sorry for the delay. Will answer your questions this afternoon asap. |
No worries, I think I forgot to request a review |
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.
This is awesome! So cool to see and use this. Requests some changes, then we can get this merged.
if (!environment) { | ||
throw new TRPCError({ | ||
code: "NOT_FOUND", | ||
}); | ||
} |
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.
You don't need this because the environmentBySlug
helper does this internally.
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.
Oh perfect!
packages/web/package.json
Outdated
@@ -57,7 +57,9 @@ | |||
"keccak": "^3.0.3", | |||
"lucide-react": "^0.354.0", | |||
"next": "^14.2.3", | |||
"prismjs": "^1.28.0", |
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 don't see this used anywhere since it appears we just copied some source code from it into our own repo. Should we uninstall 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.
Good catch. I tried to use npm and avoid including the prismjs source, but when that didn't work I forgot to remove the dep here.
packages/web/app/prism.js
Outdated
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.
Just curious why we copied this code (and the CSS) vs importing from the npm package?
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.
Prismjs seems to be optimized to be downloaded and used in this way. The npm package only include a few types of highlighting/formatting. Since we want sql highlighting I had to download the specific theme and formatting.
|
||
// splitting on "/" means there is always an empty string as the first | ||
// element in the Array since pathname always starts with / | ||
const isConsole = pathParts.length === 5 && pathParts.pop() === "console"; |
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 do have a suggestion that is a bit cleaner:
You can rely on the values returned by useParams
above to distinguish between the console
part of the route and any table name. If the user is visiting the console
page, defSlug
will be undefined
. So this give us most of the information we need to make sure we're not viewing a table (def), but we still need to distinguish between the overview page and console page which both match the case !!envSlug && !defSlug
. We can use the hook useSelectedLayoutSegments
to get a nice array of url segments below the layout without needing to split the pathname string on /
etc (as you complain about below). So the final calculation of isConsole
would be:
const isConsole =
!!envSlug && !defSlug && selectedLayoutSegments.slice(-1)[0] === "console";
|
||
// splitting on "/" means there is always an empty string as the first | ||
// element in the Array since pathname always starts with / | ||
const isConsole = pathParts.length === 5 && pathParts.pop() === "console"; |
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.
<SidebarLink | ||
icon={Terminal} | ||
title="Console" | ||
href={`/${teamQuery.data.slug}/${projectQuery.data.slug}/${linkEnv.slug}/console`} | ||
selected={isConsole} | ||
/> |
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.
This SidebarLink
should go in the SidebarSection
above along with the link for Overview. You can then delete this section.
fd34d60
to
b91dd44
Compare
b91dd44
to
2c82cfb
Compare
2c82cfb
to
ca6e068
Compare
ca6e068
to
4fbd34b
Compare
@asutula I believe I got all the change requests in here. Have another look when you get time. |
No description provided.