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

feat: add databases switcher in control panel header #257

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

belyas
Copy link

@belyas belyas commented Oct 10, 2024

This feature adds a database switcher on the top left beside the chosen database to give current user ability to switch the databases easily as we had such situation and we had to redo everything from scratch just the chosen database was not the right one.
Here how it looks like when it's open:
Screenshot 2024-10-10 104824

Here are example of different outputs based on chosen database:
Screenshot 2024-10-10 104614
Screenshot 2024-10-10 104738

Copy link

vercel bot commented Oct 10, 2024

@belyas is attempting to deploy a commit to the dottle's projects Team on Vercel.

A member of the Team first needs to authorize it.

@1ilit
Copy link
Member

1ilit commented Oct 10, 2024

Since non-generic diagrams each use data types supported by their corresponding database engines, just changing database won't cut it. In order for the output sql to be correct we need to convert the types. For example, if you have an enum column in mysql switching to postgres will result in an incorrect output

@belyas
Copy link
Author

belyas commented Oct 10, 2024

Since non-generic diagrams each use data types supported by their corresponding database engines, just changing database won't cut it. In order for the output sql to be correct we need to convert the types. For example, if you have an enum column in mysql switching to postgres will result in an incorrect output

Hi @1ilit, so the underlining export to sql won't properly do its work? because these changes have nothing to do with what the final sql would look like, I thought this use case would be as generic case, when the export to sql was clicked with the chosen database, it will trigger proper toDatabase function? what's your take on this, please?

For enum use case, I see that the current implementation doesn't properly convert enums, I got this sql for Mysql:

CREATE TABLE "table_1" (
	"id" INTEGER NOT NULL UNIQUE GENERATED BY DEFAULT AS IDENTITY,
	"response" ENUM DEFAULT yes,
	PRIMARY KEY("id")
);

It's exactly what I get when I created a fresh diagram and set same enum.
I think we can go on with this change as a feature, then we fix the underlining functions that export enums improperly, I can help with this

@belyas
Copy link
Author

belyas commented Oct 10, 2024

Actually the Enum fix for Mysql is done on #252

@belyas
Copy link
Author

belyas commented Oct 11, 2024

@1ilit can we merge this please, if you don't have any comments on the implementation? as this change will boost the UX. And regarding the underlining specific database engines' implementations have nothing to do with this feature and be done gradually in future PRs

@1ilit
Copy link
Member

1ilit commented Oct 11, 2024

@belyas sorry but this wont be merged anytime soon, it doesn't even pass the workflows. Regarding the enum issue in mysql there's already pr for it.

The issue isn't only about enums in mysql and postgres. There will be a need to provide conversions from all engines to all engines.

Feel free to deploy your fork and use that instead.

@belyas
Copy link
Author

belyas commented Oct 11, 2024

for the workflows isn't an issue, I can fix the eslint error, but I didn't get why this won't be merged as it improves the UX and ease choosing the db engines, I also don't see how this work has to do with conversions from/to engines, this has nothing to do with that, or am I wrong and I got things differently? I'd like to have a proper reply @1ilit

@belyas
Copy link
Author

belyas commented Oct 13, 2024

@1ilit I have added conversions to db engines: PostreSQL, MySQL and Sqlite and restricted the db switcher to display only these engines for now, I can add others if this PR can be merged afterwards.
Let me know what you think now

Screenshot 2024-10-13 141425
Screenshot 2024-10-13 141513

@1ilit 1ilit force-pushed the main branch 2 times, most recently from 0ca7470 to d389c28 Compare November 10, 2024 09:11
@cwst66
Copy link

cwst66 commented Dec 27, 2024

fchgv

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.

3 participants