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

Implement Dark Mode + File counter #13

Closed
wants to merge 2 commits into from
Closed

Implement Dark Mode + File counter #13

wants to merge 2 commits into from

Conversation

shalvah
Copy link

@shalvah shalvah commented Aug 22, 2022

  • Added a dark mode dropdown (I couldn't think of a good place to put it, so it's next to the logo) The switcher defaults to system setting, but will save whatever the user chooses in the session.
  • Added a file counter to show the number of files
  • Linked the "Log Viewer" main text back to the logs home.

Apologies if I missed anything; I'm not a frontend guy. Also haven't done much testing in other browsers/OSes.

Screens:

Home

image

image

image

image

@shalvah
Copy link
Author

shalvah commented Aug 22, 2022

Also, i wasn't sure whether to commit the generated app.css or not, so I just left it in.

@shalvah
Copy link
Author

shalvah commented Aug 22, 2022

Thanks again for the awesome project. Does what it says on the box. No setup required; just plain and simple.

Some ideas I think could make it even better:

  • Log text parsing? So you can specify a format like 'json' or a custom parser, and it renders your log messages nicely.
  • Auth: You probably don't want everyone to be able to access your logs. I had to create a custom admin middleware, but we could ship a very simple one that can check for the user email.

@arukompas
Copy link
Contributor

Hey @shalvah , thanks for the PR!

I have a few thoughts that hopefully make sense.

  1. Try not to include multiple features in a single PR. This makes reviewing difficult, and also impossible to approve/merge one feature without merging others with it. For example, I'm not a fan of the file counter solution - if that were a separate PR, we could just dismiss that and work on others!
  2. The actual code implementation of the Dark Mode makes sense and the code looks good! The colour choice, on the other hand, is not the most optimal. I want this package not only to work well, but also look great! I have a couple designer friends and I'll be asking them for their designs on the Dark Mode version.
  3. The "Theme" selector should instead be placed on the Settings dropdown on the right. How often will you really change the theme of the log viewer? Most will set it to "System" and forget about it, so the setting should be put away for a cleaner UI.
  4. After making js/css changes, I run npm run production to compile the assets and minify them for a much smaller bundle and faster load times for the log viewer :)

All in all, the intetion and code are great, but I think this PR lacks some UX and design flair.

I have "Dark Mode" on my todo list and will be working on it later with one of my designer friends. But in the meantime, you can publish your own fork on Packagist and require it in your project so you have Dark Mode right now 😄

@arukompas arukompas closed this Aug 23, 2022
@shalvah
Copy link
Author

shalvah commented Aug 23, 2022

Well, that's disappointing. Spent my entire morning on this. But it's your call.

For example, I'm not a fan of the file counter solution

Why? I find it extremely useful to know how many log files I have. Unless you mean the implementation. But I understand about separate PRs.

The "Theme" selector should instead be placed on the Settings dropdown on the right.

I agree, but that settings dropdown only shows up when you select a log file, and I didn't want to unilaterally change that.

After making js/css changes, I run npm run production to compile the assets and minify

Like I said, I'm not a front-end person, so I don't know all the details. (I do know about minification, but it didn't occur to me.)🥲

@shalvah shalvah deleted the dark-mode branch August 23, 2022 10:54
@@ -12,8 +12,13 @@
<style>{!! file_get_contents($cssPath) !!}</style>
@endisset
@livewireStyles
@if(session('log-viewer:theme', 'system') == 'system')
<script>
document.documentElement.classList.add('dark')
Copy link
Author

Choose a reason for hiding this comment

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

Also, I just realised there's a bug here. I forgot to add window.matchMedia("prefers-color-scheme...") thing. 🤣

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.

None yet

2 participants