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

enh: sparkle-ization of app design #1215

Merged
merged 4 commits into from
Sep 1, 2023
Merged

enh: sparkle-ization of app design #1215

merged 4 commits into from
Sep 1, 2023

Conversation

spolu
Copy link
Contributor

@spolu spolu commented Aug 31, 2023

  • Use Tab instead of subNavigation + simple title so that it can be closed
  • Rollout buttons and icons
  • Removes the old Button components

cc @Duncid I'm not super fan of the title + tab below but it's not that bad. Given that it's not our core focus I presume it's good enough but let me know if you have some ideas on that.

Screenshot from 2023-08-31 18-33-48

@spolu spolu requested a review from fontanierh September 1, 2023 10:42
@@ -67,7 +80,7 @@ export default function AI21Setup({ owner, open, setOpen, config, enabled }) {

return (
<Transition.Root show={open} as={Fragment}>
<Dialog as="div" className="relative z-10" onClose={() => setOpen(false)}>
<Dialog as="div" className="relative z-30" onClose={() => setOpen(false)}>
Copy link
Contributor

Choose a reason for hiding this comment

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

FMI, why do we need to move to z-30 everywhere ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Standardizing all Dialogs as some of them woudl conflict with the AppLayout title 👍

fontanierh
fontanierh previously approved these changes Sep 1, 2023
Copy link
Contributor

@fontanierh fontanierh left a comment

Choose a reason for hiding this comment

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

LGTM but will test quickly

@@ -290,7 +290,7 @@ export default function AppLayout({
</div>
<div
className={classNames(
"fixed left-0 right-0 top-0 z-30 flex h-16 flex-row lg:left-80",
"fixed left-0 left-12 right-0 top-0 z-30 flex h-16 flex-row lg:left-80",
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we want to remove the left-0 then no ? Because I imagine only one of those 2 can apply ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep yep

@fontanierh
Copy link
Contributor

Screenshot 2023-09-01 at 14 02 03

Super nit, but is the disabled state of the deploy button as expected ?

@spolu
Copy link
Contributor Author

spolu commented Sep 1, 2023

Yes => cc @Duncid

@spolu
Copy link
Contributor Author

spolu commented Sep 1, 2023

(@Duncid already noted that it's a bit meh meh)

@spolu
Copy link
Contributor Author

spolu commented Sep 1, 2023

Fixes dust-tt/tasks#12

@spolu spolu merged commit 32714f3 into main Sep 1, 2023
@spolu spolu deleted the spolu-apps_nav branch September 1, 2023 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants