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

Desktop Layout [WIP] #398

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Desktop Layout [WIP] #398

wants to merge 4 commits into from

Conversation

abhumbla
Copy link
Contributor

@abhumbla abhumbla commented Nov 22, 2024

current state
desktop
desktop-route

@@ -112,7 +112,7 @@ export default function DirectionsNullState(props: Props) {
values={{
link: (chunks) => (
<a
href="https://github.com/bikehopper/bikehopper-ui"
href="https://github.com/bikehopper/"
Copy link
Contributor

Choose a reason for hiding this comment

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

prefer to keep this pointed at ui for now since its readme has an overview of BikeHopper and BikeHopper's stack

I'm aware it's been argued that that isn't the correct place for an overview to live. however, currently it's the only public overview we have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i will make a more suitable public overview repo/page before merging this PR

@@ -2,7 +2,8 @@ import { MapRefs } from '../hooks/useMapRefs';
import { HtmlPortalNode, OutPortal } from 'react-reverse-portal';

type Props = {
sidebar?: JSX.Element;
header: JSX.Element;
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, I elsewhere use React.ReactNode for these. Is there an argument for JSX.Element being more correct?

@@ -0,0 +1,4 @@
.ItineraryElevation__chart {
Copy link
Contributor

Choose a reason for hiding this comment

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

Per coding conventions please use Tailwind for new components

@@ -0,0 +1,4 @@
.Routes {
Copy link
Contributor

Choose a reason for hiding this comment

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

Tailwind

@@ -16,7 +16,7 @@ import useThrottledValue from './useThrottledValue';
// first attaching to a node.

export default function useResizeObserver(
callback: (dimensions: [number, number]) => void,
callback: (dimensions: [number, number] | null) => void,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm good catch except that if I'm reading this right, is it actually [number, number] | [null, null]? (Why didn't it error?)


const _isTouch = 'ontouchstart' in window;

type Props = {
onMapLoad: () => void;
onMapLoad?: () => void;
overlayRef: RefObject<HTMLElement>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this type not match the overlayRef type in src/hooks/useMapRefs? (This type is not nullable, and could be any HTML element, not just a div.)

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.

2 participants