-
Notifications
You must be signed in to change notification settings - Fork 527
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
Ddg ui find #427
Ddg ui find #427
Conversation
Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
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.
Did you have to update something? There are a lot of changes to the yarn.lock
file.
Also, the packages were fetched from unpm.uberinternal.com
which is now in the lock file.
Signed-off-by: Everett Ross <[email protected]>
I had tried to use |
Codecov Report
@@ Coverage Diff @@
## master #427 +/- ##
=========================================
- Coverage 91.67% 91.6% -0.07%
=========================================
Files 176 176
Lines 4012 4039 +27
Branches 957 968 +11
=========================================
+ Hits 3678 3700 +22
- Misses 292 295 +3
- Partials 42 44 +2
Continue to review full report at Codecov.
|
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.
Styling looks great. LMK if any of my comments look awry.
packages/jaeger-ui/src/components/DeepDependencies/Header/index.css
Outdated
Show resolved
Hide resolved
packages/jaeger-ui/src/components/DeepDependencies/Header/index.css
Outdated
Show resolved
Hide resolved
packages/jaeger-ui/src/components/DeepDependencies/Header/index.tsx
Outdated
Show resolved
Hide resolved
packages/jaeger-ui/src/components/DeepDependencies/Header/index.tsx
Outdated
Show resolved
Hide resolved
packages/jaeger-ui/src/components/DeepDependencies/Header/index.tsx
Outdated
Show resolved
Hide resolved
@@ -55,7 +56,7 @@ export class UnconnectedUiFindInput extends React.PureComponent<TProps, StateTyp | |||
ownInputValue: undefined, | |||
}; | |||
|
|||
updateUiFindQueryParam = _debounce((uiFind: string | undefined) => { | |||
updateUiFindQueryParam = _debounce((uiFind?: string | undefined) => { |
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 think undefined
is needed when the ?
is used:
type T = { a?: string }
const t: T = { a: undefined } // ok
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.
that works but seems off imo. to me, type T
means that if Reflect.has(t, a) === true
then t.a
is a string
.
for instance:
function(pt: T) {
if (Reflect.has(pt, 'a')) {
console.log(pt.a.repeat(3));
}
}
shouldn't have any TS errors IMO, but it does (pt.a
can be undefined
so can't repeat
off of it).
Oh well, I removed the | undefined
Signed-off-by: Everett Ross <[email protected]>
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.
Added a few comments in the threads of the previous comments.
Signed-off-by: Everett Ross <[email protected]>
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.
LGTM. Great work.
Ddg ui find Signed-off-by: vvvprabhakar <[email protected]>
Adds UiFind to DDG, with temporary
isUiFindMatch
styling.Focused, with query:
![image](https://user-images.githubusercontent.com/3471184/62250173-3bcb9f00-b3ba-11e9-8011-de3f182550c7.png)
Not focused, with query:
![image](https://user-images.githubusercontent.com/3471184/62250179-4423da00-b3ba-11e9-877d-d9ac1094f670.png)
Not focused, without query:
![image](https://user-images.githubusercontent.com/3471184/62250187-49812480-b3ba-11e9-8d62-6f8fc387c5f7.png)
Unfortunately antd's
Input
inUiFindInput
made getting this right more time consuming than it should have been. Between some styling conflicts, antd'sallowClear
prop implementation (in 3.12.0+), and how it handlessuffix
it probably would've been better to simply use a nativeinput
instead.Due to other work being dependent on this, I figured it'd be best to raise this PR as is. Then we can decide the priority of finalizing these details in this PR versus wrapping up other DDG UI work.