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.

Sign up for GitHub

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

Jump to bottom

Add support for dark mode #700

Closed
wants to merge 2 commits into from
Closed

Add support for dark mode #700

wants to merge 2 commits into from

Conversation

mohamedelhefni
Copy link

this PR is addressing the feature requested at #606

dark

Changes

  • Migrate to bootstrap v5.3
  • Remove custom css that forces light theme
  • add toggle theme functionality
    • change editor theme
    • change the data-bs-theme attribute

@sosedoff
Copy link
Owner

sosedoff commented Jan 5, 2024

Can you break out bootstrap upgrade into a separate PR? That'll be something we would need to merge first and is definitely easier to check/verify independently from the other changes you're proposing.

sosedoff
sosedoff reviewed Jan 5, 2024
View reviewed changes
static/index.html
<link rel="stylesheet" href="static/css/bootstrap.css"></link>
<link rel="stylesheet" href="static/css/font-awesome.css"></link>
<link rel="stylesheet" href="static/css/app.css"></link>
<link rel="stylesheet" href="static/css/bootstrap.min.css">
Copy link
Owner

Choose a reason for hiding this comment

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

Keep the old formatting here

static/index.html
@@ -35,14 +40,17 @@
</ul>

<div class="connection-actions">
<a href="#" id="edit_connection" class="btn btn-default btn-sm"><i class="fa fa-database"></i> Connect</a>
<a href="#" id="close_connection" class="btn btn-default btn-sm">Disconnect</a>
<a href="#" id="edit_connection" class="btn btn-default text-white btn-sm"><i
Copy link
Owner

Choose a reason for hiding this comment

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

Are you using some kind of auto formatter for html here? The tag content looks broken over the lines

static/js/app.js
var bookmarks = {};
var default_rows_limit = 100;
var currentObject = null;
var appInfo = {};
Copy link
Owner

Choose a reason for hiding this comment

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

Please keep the original formatting in this file

@mohamedelhefni
Copy link
Author

Hi @sosedoff
I have a strong interest in the pgweb project.
Initially, I noticed the absence of vim mode keybindings, which prompted me to contribute to the project. While working on it,
I identified an issue related to adding support for a dark theme. Initially, I thought upgrading Bootstrap to v5.3 would seamlessly enable dark theme support, but this process ended up causing complications.
The upgrade adversely affected elements like the context menu and dropdown menu, and since the context menu plugins are no longer actively maintained, resolving these issues would take more time than anticipated.
Regrettably, I have decided to close this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sosedoff sosedoff sosedoff left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mohamedelhefni @sosedoff