-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add a feature allowing for custom filenames. #58
Conversation
Ensure no merge conflicts with main
site/package.json
Outdated
"@mui/icons-material": "^5.15.19", | ||
"@mui/material": "^5.15.18", |
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 idea of the impact on this change on our bundle size? I hesitate to introduce Material as a dependency unless we are going to use a lot of their components.
We currently are using Infima for styling and components, but looks like a popover is missing. I personally am a fan of https://www.radix-ui.com/primitives/docs/components/popover because you can selectively import the components you use (e.g. https://github.com/placemark/placemark/blob/main/package.json#L58).
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 could definitely transition to this. I've used Material in the past, so it is what I know, but if Radix provides more 'customizable' importing I could switch to that.
site/src/nav/DownloadButton.jsx
Outdated
downloadLink.download = `${fileName}.geojson`; | ||
} | ||
|
||
// downloadLink.download = `overture-${zoom}-${center.lat}-${center.lng}.geojson`; |
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'd prefer if we leave commented out code out of main
.
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.
Definitely will do this, there is some other dead code that can be removed as well.
site/src/nav/DownloadButton.css
Outdated
} | ||
|
||
div.ffw-dark { | ||
background: dimgray; |
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.
Can we use the Infima variable for the navbar here?
This grey is lighter and provides less contrast for dark mode. Using shared CSS variables as much as possible will lead to a more consistent UI.
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 can, I think I was more hesitant too match the navbar color completely. I wasn't 100% if the style we wanted to use for dark mode was to match the navbar, so I decided to make it a noticeably different color to differentiate it. I will change it to match now.
Here is a demo of the updated file-naming option box. The background and overall colorings should better match the rest of the site now, and it has switched from Material to Radix to decrease unneeded dependencies. Screen.Recording.2024-06-03.at.3.25.23.PM.mov |
This work is going in a good direction- after discussing, @eti7075 and I have decided to shelve the 'download options' and move to a more general-purpose settings / preferences panel. |
After discussion, it seems that a custom filename option is more preferable than the zoom/at/lng filename that was previously implemented. This feature provides a dropdown that allows for the user to enter a custom filename, or opt not too, then click a
confirm
button that will initiate the download. The default filename, if none is provided, isoverture.geojson
. A video demoing this is provided below.Screen.Recording.2024-05-31.at.2.02.27.PM.mov