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

Styling additions #1

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Frischid
Copy link
Contributor

Hi @EinEinfach,

first of all, thank you very much for this great app, it's awesome.
I had some spare time but no access to our mower, so I implemented some ideas to improve some parts (in my opinion).
If you don't like them just ignore or close the merge request.

  1. since I like tooltips as a quick explanation, I added title attributes to the button which are displayed as tooltip
  2. the bootstrap themes are loaded from a CDN, which could cause issued for offline solutions -> I added the css and font files to the assets folder and set the correct path in app.py
  3. in my opinion the map is the main part of the app, additional containers and a flex layout should always fill the remaining layout with the map
  4. moving the remote control button and info button to the header gives access to them with less clicks
  5. Adding a webmanifest and logo enables e.g. android users to add the app to their homescreen and lunch it as a standalone app, simply change favicon.ico with your favorite icon, I added just a simple one

All the additions are mainly styling and personal, if you don't like them that's fine, I won't be mad at you :)

cassandra

@EinEinfach
Copy link
Owner

Hello Dominik,

very good improvements. I like your suggestions to move info and control buttons to the navbar, also is that very good idea to use icons and stylesheet offline.

I have to check how to merge your changes with my uncommitted changes. Maybe also good idea for me to implement your features manually to better understand your code changes :) Style stuff is not really what I can. Maybe I can learn something

@Frischid
Copy link
Contributor Author

To be honest, styling is also not something I really like nor am good at, but I had to deal with bootstrap in small html/css projects before, so I was familiar with one of the basic ideas in bootstrap (https://getbootstrap.com/docs/5.0/layout/grid/)

But I've never used dash before, so I mostly added the classes to className instead of using the corresponding python parameter. Additionally, dash adds a few div which wouldn't be needed and could even break things but display: contents should have fixed that (https://caniuse.com/css-display-contents).
Let me know if you want to know the reason for any change, I'll be happy to explain.

@EinEinfach
Copy link
Owner

EinEinfach commented May 12, 2023

Hello Dominik,
I checked your changes locally, but it seems to be that bootstrap themes are not loaded (or not loaded correctly)

photo1683878658

@EinEinfach
Copy link
Owner

EinEinfach commented May 12, 2023

I changed in the app.py
assets_path = os.getcwd() +'/src/assets'
to
assets_path = os.path.dirname(file) + '/src/assets'

seems to be working

EinEinfach added a commit that referenced this pull request May 12, 2023
Suggetions from pull-request:
#1
EinEinfach added a commit that referenced this pull request May 12, 2023
- The bootstrap themes are not more loaded from CDN
- Theme is available offline
- Suggestion from pull-request:
#1
Thanks to user Frischid!!!
EinEinfach added a commit that referenced this pull request May 12, 2023
- Component simple navbar replaced through navbar
- Remote control and infobox button moved out to navbar to reduce amount of clicks
- Sugestion from pul-request: #1
- Thanks to user Frischid!!!
@Frischid
Copy link
Contributor Author

assets_path = os.path.dirname(__file__) + '/src/assets'

That's perfect, wasn't aware of __file__ that seems to be much better and independent of the cwd now 👍

@EinEinfach
Copy link
Owner

Hello Dominik,

I implemented almost changes from your pull request. I think just changing to doc.Containers is open. But now I see one problem, I can't scroll content in browser (just saw it on mobile safari, don't if other browsers also affected). Do you know what could be a problem?

@EinEinfach
Copy link
Owner

Ok, I got it!

@Frischid
Copy link
Contributor Author

Hi Alexander,
sorry for not being reachable during the day. Not being able to scroll is intended to some extend, to make the app fullscreen and the map size should adapt in my pull request, so there is no need to scroll then, that was the idea basically.
I think you did not yet implement the map with the variable height but the fixed height container, which resulted in the observed issue on small devices. The map height would shrink in my PR and make enough space for everything to be visible without scrolling. To get a larger map on small devices, I would rather think about sth like this https://stackoverflow.com/a/69096923 than enabling scrolling, which could be confusing when "scrolling" in the middle of the map, which could scroll the map but you wanted to scroll the page.. (if interested I could dig into that and try to add it in a small commit)
Adding the overall container around the pages should help to get rid of the left/right scrolling.
You could also reduce the height of the navbar on small devices by reducing the margin of button-open-modal-info (ms and mt https://getbootstrap.com/docs/5.0/utilities/spacing/), I just saw that could be a little bit much.

@Frischid Frischid mentioned this pull request Jul 12, 2023
@holgerschochwitz
Copy link

holgerschochwitz commented Apr 21, 2024

Hallo,
bezüglich der Button, wäre es möglich die Umschaltbutton in einer anderen Farbe darzustellen, wenn sie gedrückt sind. Der Button zur Auswahl von Dockpunkten wird dunkler, behält aber die Farbe. Bei ungünstigem Licht, sieht man den Unterschied nicht.
Wäre die Def
--bs-primary-active:rgb(247, 50, 103);
in ...assets/style.css

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.

3 participants