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

Web: added connections and databases selector, changed theme color #302

Merged
merged 44 commits into from
Jan 15, 2025

Conversation

liuliu-dev
Copy link
Contributor

@liuliu-dev liuliu-dev commented Nov 21, 2024

@liuliu-dev liuliu-dev requested a review from tbantle22 January 3, 2025 19:26
@liuliu-dev liuliu-dev changed the title Web: add connections and databases selector to desktop nav Web: added connections and databases selector, changed theme color Jan 3, 2025
Copy link
Collaborator

@tbantle22 tbantle22 left a comment

Choose a reason for hiding this comment

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

Code LGTM, still reviewing UI

Comment on lines 50 to 72
<Button.Link
key={conn.name}
className={cx(css.connection, {
[css.selected]: state.connection.name === conn.name,
})}
onClick={async () => onSelected(conn)}
>
<div className={css.connectionTop}>
<div className={css.nameAndLabel}>
<span className={css.connectionName}>
{excerpt(conn.name, 16)}
</span>
<DatabaseTypeLabel conn={conn} />
{conn.name === currentConnection.name && (
<MdRemoveRedEye className={css.viewing} />
)}
</div>
<FaChevronRight className={css.arrow} />
</div>
<div className={css.connectionUrl}>
{excerpt(getHostAndPort(conn.connectionUrl), 42)}
</div>
</Button.Link>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would make this easier to read by moving this to a ConnectionItem component

}
return (
<Button.Link
className={cx(css.DbItem, css.link)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
className={cx(css.DbItem, css.link)}
className={cx(css.dbItem, css.link)}

export function DatabaseTypeLabel({ conn }: Props) {
const type = getDatabaseType(conn.type ?? undefined, !!conn.isDolt);
switch (type) {
case "Dolt":
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is type not an enum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, it's a string returned from getDatabaseType

Comment on lines 31 to 33
className={cx(css.titlebar, {
[css.drag]: !noDrag,
[css.noDrag]: noDrag,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you make .drag part of .titlebar since that's the default state?


return (
<div
className={cx(css.titlebar, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
className={cx(css.titlebar, {
className={cx(css.titleBar, {


export function Logo({ imgSrc, className }: LogoProps) {
return (
<Link href="/" className={cx(className)}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<Link href="/" className={cx(className)}>
<Link href="/" className={className}>

@@ -15,11 +16,13 @@ export default function useDatabaseDetails(): ReturnType {
const isDolt = res.data?.doltDatabaseDetails.isDolt ?? false;
const isPostgres =
res.data?.doltDatabaseDetails.type === DatabaseType.Postgres;
const isMysql = res.data?.doltDatabaseDetails.type === DatabaseType.Mysql;
Copy link
Collaborator

Choose a reason for hiding this comment

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

You shouldn't need this. If both isDolt and isPostgres are false then it's a mysql db

Comment on lines 41 to 45
await res.client.clearStore();
if (!addedDb.data) {
return;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should clearStore only be called if addedDb.data exists?

if (conn.type === DatabaseType.Postgres) {
await resetDB({ variables: { newDatabase: databaseName } });
}
const { href, as } = database({ databaseName });
const { href, as } = maybeDatabase(databaseName);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why maybeDatabase? Shouldn't databaseName always exist?


type Props = {
conn: DatabaseConnectionFragment;
state: StateType;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you need to pass the whole state? Seems like you only need connection (which could use a clearer name to distinguish from conn)?

@liuliu-dev liuliu-dev merged commit c050456 into main Jan 15, 2025
1 check passed
@liuliu-dev liuliu-dev deleted the liuliu/web-nav-dropdown branch January 15, 2025 22:23
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.

2 participants