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

ui: Fix fuzzy search namespace-handling #10666

Merged
merged 6 commits into from
Aug 10, 2021
Merged

Conversation

backspace
Copy link
Contributor

The initial UI for fuzzy search failed to include the flag to query across all namespaces. This also has a suggested implementation for showing namespaces in job and allocation label like job @ namespace, but this is perhaps not an established pattern in the UI, and maybe other search results should have namespaces too, maybe it should be lighter grey…? Things to consider 🤔

@github-actions
Copy link

github-actions bot commented May 27, 2021

Ember Asset Size action

As of 00df085

Files that got Bigger 🚨:

File raw gzip
nomad-ui.js +58 B +14 B

Files that stayed the same size 🤷‍:

File raw gzip
vendor.js 0 B 0 B
nomad-ui.css 0 B 0 B
vendor.css 0 B 0 B

@github-actions
Copy link

github-actions bot commented May 27, 2021

Ember Test Audit comparison

main 2c30c336d20ece0d43d62f6409d4fde6e7843170 change
passes 1107 1107 0
failures 0 0 0
flaky 0 0 0
duration 9m 06s 651ms 8m 30s 908ms -35s 743ms

@tgross
Copy link
Member

tgross commented Jun 3, 2021

@JBhagat841 I'm assigning this to you just so that we don't lose track of open PRs from Buck, but there's no special rush on it other than that we should try to ship it in the 1.1.x cycle.

@davemay99 davemay99 added the hcc/cst Admin - internal label Jul 6, 2021
Namespaces are set-up in Nomad to be an object that has an id property.
However, namespaces actually don't have that shape. Our search was expecting
a namespace object, but we actually don't have a namespace assigned to jobs
in our config and namespace is set to null. Normally, these namespaces would
be set to default, but that would require us to refactor our Mirage config
if we wanted to assert that namespaces are 'default' and not null. So this is
a bandaid solution.
@ChaiWithJai ChaiWithJai force-pushed the b-ui/search-namespaces branch from e354f68 to 2649d19 Compare July 26, 2021 21:25
@vercel vercel bot temporarily deployed to Preview – nomad July 26, 2021 21:25 Inactive
@ChaiWithJai ChaiWithJai requested a review from lgfa29 August 5, 2021 19:22
@lgfa29 lgfa29 marked this pull request as ready for review August 5, 2021 21:48
Copy link
Contributor

@lgfa29 lgfa29 left a comment

Choose a reason for hiding this comment

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

Minor comments, but overall looking great!

I noticed an issue when the search bar loses focus where its background color switches back to green, making it look a bit odd:
image

This was not caused by this PR though, so it can be fixed later 👍

ui/app/components/global-search/control.js Outdated Show resolved Hide resolved
ui/app/components/global-search/control.js Outdated Show resolved Hide resolved
Copy link
Contributor

@lgfa29 lgfa29 left a comment

Choose a reason for hiding this comment

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

Looking good! Just missing a changelog entry 😉

@ChaiWithJai ChaiWithJai force-pushed the b-ui/search-namespaces branch from 2c30c33 to 00df085 Compare August 10, 2021 14:35
@vercel vercel bot temporarily deployed to Preview – nomad August 10, 2021 14:35 Inactive
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants