-
Notifications
You must be signed in to change notification settings - Fork 13
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
Darkmode #67
base: main
Are you sure you want to change the base?
Darkmode #67
Conversation
- Add dark mode configuration to localStorage with auto-apply - Add dark mode toggle checkbox in Settings page - Implement dark mode styles for: - Main app container and navigation - Tables in AllFlights page with hover states - Text and background colors - Configure Tailwind for dark mode using class strategy
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 just quickly glanced over your changes, overall it's looking pretty good, though I haven't tested it myself yet. I only had some minor remarks to make, thanks a lot for the contributions
@@ -5,3 +5,5 @@ __pycache__ | |||
package-lock.json | |||
.parcel-cache | |||
*.old | |||
jetlog.db | |||
run_jetlog.sh |
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.
Since this line is specific to your dev environment, I don't think it belongs in the Github repo. I'm fine with the jetlog.db
line though, which can be handy.
case "default": | ||
default: | ||
colors = "bg-white text-black border border-gray-300 enabled:hover:bg-gray-100"; | ||
onClick?: (() => void) | null; |
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.
Any reason why the type of this function was changed?
<textarea rows={5} | ||
className="w-full px-1 mb-4 bg-white rounded-none outline-none font-mono box-border | ||
border-2 border-gray-200 focus:border-primary-400" | ||
<textarea className="w-full px-3 py-2 mb-3 |
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.
Is it possible to re-insert the rows
attribute here?
type="checkbox" | ||
name={name} | ||
checked={checked} | ||
onChange={onChange ? (e: React.ChangeEvent<HTMLInputElement>) => onChange(e) : 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 feel like the type declaration on e
is redundant here, given its context
} | ||
|
||
interface OptionProps { | ||
text: string; | ||
value?: string; | ||
} | ||
function Option({text, value}: OptionProps) { | ||
export function Option({text, value}: OptionProps) { |
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 this has to be exported, since it's only ever used for the Select
component, right?
} | ||
} | ||
} | ||
|
||
const handleExportClick = (exportType: string) => { | ||
if(exportType === "csv") { | ||
if (exportType === "csv") { |
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 the sake of consistency with the rest of the code base, there shouldn't be spaces after if
, for
, ...
The same goes for other parts of the code in this PR where a space was added after such keywords
No description provided.