Skip to content
This repository has been archived by the owner on Feb 8, 2024. It is now read-only.

Teleport Connect: Add dropdown for database name #757

Merged
merged 10 commits into from
Apr 26, 2022
Merged

Conversation

ravicious
Copy link
Member

@ravicious ravicious commented Apr 25, 2022

Adds a second dropdown for the db name. Needs tsh compiled with changes from gravitational/teleport#12173.

This will be needed for the db name dropdown, where the value is optional.
Comment on lines 61 to -56
flex="1"
width="100%"
Copy link
Member Author

Choose a reason for hiding this comment

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

flex: 1 and width: 100% seem unecessary. Instead of taking 100% width of its parent and letting other elements cut into it (like ConnectionStatusIndicator did here), let's use flexbox to its fullest: make ConnectionStatusIndicator never shrink and make this div here take all remaining space with flex: 1.

minWidth="0"
>
<Flex flexDirection="column">
Copy link
Member Author

Choose a reason for hiding this comment

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

Block elements by default behave like flex with direction set to column, so I realized that this one here is unnecessary.

Comment on lines +64 to +68
<div
css={`
min-width: 0;
`}
>
Copy link
Member Author

Choose a reason for hiding this comment

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

There's a really nice answer on SO about it. Flex children by default have min-width set to auto, so text-overflow will never work on them.

Instead, we have to, in a cascading fashion, set min-width on those flex children which have content that should get hidden on overflow. So we have min-width on this div here and one above it.

<Text
typography="body1"
bold
color="text.primary"
title={props.item.title}
css={`
line-height: 16px;
white-space: normal;
Copy link
Member Author

Choose a reason for hiding this comment

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

white-space: normal was causing the UI to behave inconsistently. Document titles or cluster names with hyphens in them were getting split on multiple lines with no ellipsis, while the same elements without hyphens were cut and an ellipsis was added.

Comment on lines +17 to +24
await waitFor(async () =>
fireEvent.keyPress(await findByPlaceholderText('MenuLogin input'), {
key: 'Enter',
keyCode: 13,
})
);

expect(onSelect).toHaveBeenCalledTimes(0);
Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't sure whether to put waitFor on fireEvent vs expect().toHaveBeenCalledTimes(0).

My understanding was that in order to verify that something was not at all, waitFor would have to wait until its default timeout runs out. To verify if fireEvent was called, it only needs to check if the event was fired.

Turns out this is not how waitFor works at all. If the callback doesn't return a promise, waitFor just continuously calls the callback until it stops throwing an error or the timeout runs out.

Anyway, I guess it's more readable to have expectations without waitFor.

Copy link
Contributor

@gzdunek gzdunek left a comment

Choose a reason for hiding this comment

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

Looks good!

@@ -24,7 +24,7 @@ export function useDatabases() {
const dbs = clusterContext.getDbs();
const syncStatus = clusterContext.getSyncStatus().dbs;

function connect(dbUri: string, user: string): void {
function connect(dbUri: string, dbUser: string, dbName: string): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

When having 2+ params, it's better to convert them to object

@ravicious ravicious merged commit f12a4fc into master Apr 26, 2022
@ravicious ravicious deleted the ravicious/db-name branch April 26, 2022 09:40
ravicious added a commit that referenced this pull request Apr 26, 2022
* Add required prop to MenuLogin

This will be needed for the db name dropdown, where the value is optional.

* Update proto files

* Include targetSubresourceName when creating a gateway

* Set better title for gateway tab

* Fix long document titles breaking connection tracker's UI
ravicious added a commit that referenced this pull request Apr 27, 2022
* Add required prop to MenuLogin

This will be needed for the db name dropdown, where the value is optional.

* Update proto files

* Include targetSubresourceName when creating a gateway

* Set better title for gateway tab

* Fix long document titles breaking connection tracker's UI
ravicious added a commit that referenced this pull request Apr 27, 2022
* Bring back native scrollbar as the styled one causes content to jump when it becomes visible

* Use new colors for theme

* Fix not clickable notifications when displayed over xterm

* Close `Identity` popover after selecting an option (#741)

* Fix getting cwd in presence of lsof warnings (#745)

lsof uses stderr to print out warnings. This caused the previous version
of ptyProcess to throw an error, even though the exit code of lsof was 0.

The existing code already handles non-zero exit codes (asyncExec will
reject the promise). Since we don't know why stderr was inspected in
the first place, let's just remove the check for it.

* Change connections shortcut to `Command/Ctrl-P` (#747)

* Resolve issues on logout (#740)

* Change app name to `Teleport Connect` (#753)

* Show database username suggestions in Teleport Connect (#754)

* Move useAsync to the shared package, add docs

* Add async variant of MenuLogin

* Add Teleport Connect version of MenuLogin to its story

* Update gRPC files

* Show database username suggestions

* Use JSdoc in useAsync

Co-authored-by: Grzegorz Zdunek <[email protected]>

* Convert useAsync to a named export

* Refactor MenuLogin to use useAsync underneath

* Rewrite useAsync to use a promise instead of async

The Web UI currently doesn't support `async`. To do this, we'd need to
configure polyfills there, but we don't have time for that right now.

* Replace async with promise in MenuLogin

Co-authored-by: Grzegorz Zdunek <[email protected]>

* Fix check for the --insecure flag (#758)

Fixes gravitational/webapps.e#147.

* Remove state related to a cluster when removing it (#755)

* Teleport Connect: Add dropdown for database name (#757)

* Add required prop to MenuLogin

This will be needed for the db name dropdown, where the value is optional.

* Update proto files

* Include targetSubresourceName when creating a gateway

* Set better title for gateway tab

* Fix long document titles breaking connection tracker's UI

Co-authored-by: Grzegorz Zdunek <[email protected]>
Co-authored-by: Grzegorz Zdunek <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants