Skip to content

17 make the website mobile friendly#27

Open
Vicguin65 wants to merge 16 commits intomainfrom
17-make-the-website-mobile-friendly
Open

17 make the website mobile friendly#27
Vicguin65 wants to merge 16 commits intomainfrom
17-make-the-website-mobile-friendly

Conversation

@Vicguin65
Copy link
Contributor

Hey Matt,

If you have time/energy, are you willing to review this pull request? This pull request is relatively big. This is a very new experience for me and Ritika and we were wondering if you would give us lots of advice? We were not familiar with best practices and design so please give whatever feedback.

@Vicguin65 Vicguin65 requested a review from MattLMerritt April 5, 2024 21:47
@Vicguin65 Vicguin65 linked an issue Apr 5, 2024 that may be closed by this pull request
const BuildingDropdown = ({ place_holder_text, building_options, setSelectedBuilding }) => {
// component for allowing a user to select a building from building options

const handleChange = (selectedOption) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this function name should be changed to be more descriptive/less general.

const [displayingRoute, setDisplayingRoute] = useState(false);
const [foundRoute, setFoundRoute] = useState(false);

const [toggle, setToggle] = useState(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this hook's name should be updated to be more descriptive of what it's toggling

onChange={handleChange}
options={building_options} />
options={building_options}
styles={{
Copy link
Contributor

Choose a reason for hiding this comment

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

Microsoft edge has a 3d view tool in it's developer tools that may be helpful in viewing/updating your zIndex changes. I recommend looking for other approaches for setting the Zindex dynamically/setting these values to more "reasonable" values.

Copy link
Contributor

@MattLMerritt MattLMerritt left a comment

Choose a reason for hiding this comment

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

Incredible work! I've left a few nits related to good practices, but it LGTM.

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.

Make the website mobile-friendly

3 participants