-
Notifications
You must be signed in to change notification settings - Fork 196
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: AppNav improvements #231
Conversation
🦋 Changeset detectedLatest commit: b43953a The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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 great, couple comments. If we're using local storage we could think about extending this to store most recent searches or something too!
packages/app/src/AppNav.tsx
Outdated
@@ -514,6 +517,27 @@ export default function AppNav({ fixed = false }: { fixed?: boolean }) { | |||
: 'ok' // All alerts are green | |||
: 'none'; // No alerts are set up | |||
|
|||
const [searchesListQ, setSearchesListQ] = useState(''); |
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.
Could consider using local storage here and for dashboardsListQ
, too.
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.
was thinking about it, but not sure if keeping the search query after page refresh it a good ux. on the other hand, there's this bug with AppNav being completely re-mounted and resetting everything when moving between pages – i'm going to address it separately, should be something with next.js templates
packages/app/src/AppNav.tsx
Outdated
return logViews; | ||
} | ||
return logViews.filter(lv => | ||
lv.name.toLocaleLowerCase().includes(searchesListQ.toLocaleLowerCase()), |
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.
Nit: These are probably a candidate for future refactoring into a function since they look identical except the logViews
and searchesListQ
parameter differences 👍
packages/app/src/AppNav.tsx
Outdated
/> | ||
{dashboards.length === 0 ? ( | ||
<div className="text-slate-300 fs-8 p-2 px-3"> | ||
No saved dashboards |
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.
Nit: Should probably match the No results matching <i>{searchesListQ}</i>
style above
e74a92a
to
a21d65e
Compare
a21d65e
to
7dce8e7
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 great! Only feedback is on the new saved search empty state
packages/app/src/AppNav.tsx
Outdated
if (q === '') { | ||
return items; | ||
} | ||
return items.filter(item => |
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.
just an fyi, we do have fuse.js already included and can use it in the future as well for fancy search :)
packages/app/src/AppNav.tsx
Outdated
> | ||
<div className="d-inline-block text-truncate"> | ||
{lv.name} | ||
<div className="mt-1 "> |
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.
Just dup'ing the same message in chat, not totally sold on using Live Tail
for empty state, I think ideally we still try to hint to the user as much as we can that they can create saved searches.
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.
sounds good, added the empty states back
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.
🐑it
Screen.Recording.2024-01-11.at.11.34.47.PM.mov
Upd: