Changed the app logo (favicon) #103 and Dark/light Switch added #74 #110
Conversation
Scharfcsh
changed the title
There was a problem hiding this comment.
Thanks for opening a PR!
First of all kudos to translating some stuff into bosnian, but just for future reference we all understand english :D Also thanks for taking time to try to tackle all these issues.
Now to the feedback. There are a few general points and code specific ones you can find in the code sections below.
- The image below shows my result after spinning up your PR. The default mode is light (no problem there) but the integration of the other components (for example the footer) or the datepicker text isn't complete. Be sure to watch out for merging of Convert datepicker to shadcn/ui component #108 and Convert station selection comboboxes to shadcn/ui #109 (should happen in the next hour or so) so then it won't be necessary to style that MUI date picker text that is in your version. This light mode is unnacceptable of course so changes are required at least for that.
2, Please create a new branch on your fork when opening a PR so maintainers and other contributors can add commits to the PR. This will also enable the possibility of doing 3.
- Opening a 1 PR for two existing issues isn't best practice since I can't merge only the icon for example. Opening two PRs would be much better.
All in all, I'd suggest you scratch this PR and open new ones while implementing the feedback I left for you in this, as well as in the code comments below.
The image referenced by 1.
Once more, thanks for taking the time to make a contribution! Looking forward to the corrections! Let me know if you need further guidance.
| @@ -1,6 +1,8 @@ | |||
|
|
|||
| node_modules | |||
| .env | |||
| .env.production | |||
| .env.local | |||
There was a problem hiding this comment.
These are unecessary because of line 30 below
There was a problem hiding this comment.
the changes to this file all seem to be related to code formating
we don't have a strict ruleset for formatting yet but just for your reference this would ideally go into another commit instead of this one which points to just an icon change
no changes needed
frontend/src/app/layout.tsx
Outdated
| @@ -4,6 +4,7 @@ import { Providers } from "./providers"; | |||
| import Footer from "./components/Footer"; | |||
| import { Toaster } from "./components/ui/toaster"; | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
why the extra line here?
There was a problem hiding this comment.
It's because of my IDEs formatter , and I completely missed out that part. I'll keep that in mind from now on.
There was a problem hiding this comment.
from the second commit I see a complete replacement of the file which doesn't at all indicate what specifically was changed, formatting specific parts is not problematic like in the previous commit but this is now more difficult to review since everything is marked as changed..
There was a problem hiding this comment.
Shadcn documentation suggested to use <ThemeProvider> and keep children inside it.
So I wrapped <Provider> inside <themeProvider>.
There was a problem hiding this comment.
the same comment as the 2nd comment for RouteSearch.tsx
There was a problem hiding this comment.
thank you for feedback. I will make sure to avoid these unnecessary changes.
#103
Replaced the default favicon.ico in "frontend/src/app" logo to icon.png(it's the same image given ZebusezLogo.png).
And deleted vercels and next defaults icons.
Zamijenjen default favicon.ico u logotipu "frontend/src/app" u icon.png (to je ista slika data ZebusezLogo.png).
I obrisane vercele i sljedeće zadane ikone.
#74 Dodan je prekidač za svjetlo/tamno
@bambooch