-
-
Notifications
You must be signed in to change notification settings - Fork 338
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
New DB connection form #3319
New DB connection form #3319
Conversation
f033cb4
to
a8540fb
Compare
4fc42cc
to
3a0db6b
Compare
- Remove ConnectionModel class since we're not using it heavily. - Store connections in a map instead of an array. - Sort connections by nickname. - Compare connections host and port as well when determining whether two connections point to the same database. - Remove unused requestStatus property.
8225cc5
to
d4c0fa0
Compare
@ghislaineguerin @kgodey @pavish @mathemancer since (I think) you were all involved in reviewing the spec I want to give you a heads up about a minor design change that I improvised within this PR. See the "Connection nickname displays in connection selector" section above and let me know if you have any concerns. |
No concerns. |
No concerns from me. |
@seancolsen looks good. no concerns |
@pavish this is ready for further review now. |
c30b99f
to
5594548
Compare
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.
Looks good!
Fixes #3311
Discrepancies from spec
This PR follows the spec except in these places:
Connection nickname displays in connection selector
I decided to display the connection nickname to the connection selector dropdown:
This makes the options easier to read and has the added benefit of giving us a easy way to distinguish the internal connection from the user database connections.
"Learn more" docs hyperlinks missing
The spec has some "Learn more" hyperlinks inside help text which should point to docs pages. I'd like to add those in a subsequent PR, potentially even in a subsequent release. I'll open an issue for it after we merge this PR and we can discuss priority at that point.
Checklist
Update index.md
).develop
branch of the repositoryDeveloper Certificate of Origin
Developer Certificate of Origin