-
Notifications
You must be signed in to change notification settings - Fork 53
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
Adding new design+new download functionality #13
base: master
Are you sure you want to change the base?
Conversation
@lucifercr07 can you review |
<div className="border rounded-lg p-4"> | ||
<h2 className="text-xl font-semibold mb-4">Recent Commands</h2> | ||
<ul className="space-y-2 border"> | ||
{['SELECT * FROM users', 'INSERT INTO products', 'UPDATE orders SET status = "shipped"'].map((cmd, index) => ( |
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.
Why's this required?
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.
We can use the browser session to populate recently run commands. I've just kept a placeholder array for now. Do you think this feature is worth implementing?
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.
For now I'm not sure, let's create an issue out of it and we can pick this up later after we go live.
import Cli from "@/components/CLI/CLI"; | ||
import SearchBox from "@/components/Search/SearchBox"; | ||
import { Dice1, Dice3, Dice5 } from "lucide-react"; | ||
import { Dice1, Dice3, Dice5, Clock, Command, Sun, Moon, Github, Download } from "lucide-react"; |
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.
Let's remove the download functionality for now, please open an issue for this we can pick up later.
|
||
|
||
const NavBar: React.FC<{ isDarkMode: boolean; toggleDarkMode: () => void }> = ({ isDarkMode, toggleDarkMode }) => ( |
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.
We want to keep things in parity with our website dicedb.io, please open an issue for DarkMode we can take this up later.
@saiphanindra1010 thanks a lot for your contribution. I'd suggest let's just limit the scope of this PR to Cleanup and Commands left for now. Also please contribute test cases for components, may be as a separate PR, we're eagerly looking for that. |
@lucifercr07 do I close pr I see one more pr which is fixing ui |
@saiphanindra1010 we can keep your header changes and cleanup timer, commands left changes. Rest we can create an issue and fix. |
@lucifercr07 done removed download and hardcoded data |
@saiphanindra1010 recent commands is also not required for now I believe. We can add these functionalities later. Also as mentioned in above comment please align docs, blog etc. in header to be in parity with dicedb.io website. |
@lucifercr07 All the links are pointing to the correct links and reverted the commends |
@lucifercr07 in lucid-react brand icons are deprecated do I use a different package? |
this is how it looks without logos cc @lucifercr07 |
@saiphanindra1010 please fix the merge conflicts. |
Added new features and improvements:
Before:
After: