-
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
Equity maps; various other UI enhancements #141
Conversation
davemfish
commented
Sep 6, 2024
•
edited
Loading
edited
- Added layers displaying bivariate equity variables: heat-income and heat-race.
- Added a new landing page displaying this layer, its legend, and some text descriptions of the application
- Added some state to control which map layers are displayed when, including better defaults for first-time visitors & return visitors
- Various other style cleanup
- Various bugfixes with interactions in the study area table, etc
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.
Thanks @davemfish, I had a few questions and comments. I know we're kind of playing these fast and loose a bit, so let me know what you think the right level of review is here. It was hard to have a full understanding without seeing the workflow in person / having screenshots.
const [servicesheds, setServicesheds] = useState({}); | ||
const [activeTab, setActiveTab] = useState('explore'); | ||
const [start, setStart] = useState(0); |
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.
What's start
tracking?
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.
Tracking if the "Get Started" button was clicked.
frontend/src/edit/edit.jsx
Outdated
} = props; | ||
|
||
const [activeTab, setActiveTab] = useState('scenarios'); | ||
// const [activeTab, setActiveTab] = useState('explore'); |
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.
Looks like this was up into App.jsx
@@ -67,10 +71,18 @@ export default function App() { | |||
setPatternSamplingMode((mode) => !mode); | |||
}; | |||
|
|||
const startBuilding = () => { |
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.
Could be useful to have a docstring / comment for the purpose of this function.
@@ -5,6 +5,7 @@ import { | |||
InputGroup, | |||
Radio, | |||
RadioGroup, | |||
Section, |
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.
This was added, but does it get used?
}, | ||
); | ||
}; | ||
|
||
useEffect(() => { | ||
const center = [-10968819.475036152, 3423289.328458109]; | ||
if (start) { |
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.
Okay, so start
is driving the "walkthrough" animations?
@@ -345,6 +409,7 @@ export default function MapComponent(props) { | |||
); | |||
|
|||
map.on(['click'], async (event) => { | |||
// console.log(map.getCoordinateFromPixel(event.pixel)); |
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.
Leftover logging?
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 thought it is somewhat likely I will want to use this again in the future, so decided to leave it in for convenience.
Yeah I completely agree. Don't feel like you need to devote too much time. And feel free to pull the fork and try it out! Good idea to add some comments and clean things up. I pushed some changes. |
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.
Thanks Dave, sounds good to me!