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

refactoring and updates #7

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ForsakenHarmony
Copy link

Got a bit sidetracked, but this should fix #1 and #3

Closes #1
Closes #3

function App() {
const rerender = useRerender()

useEffect(() => {
Copy link
Member

Choose a reason for hiding this comment

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

What benefit does this change bring? (Genuinely asking.)

Copy link
Author

Choose a reason for hiding this comment

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

Probably none, was looking into putting all the life cycle into preact, but I'll revert this as well

@derhuerst
Copy link
Member

Hey, thanks for taking the time to fix #1 and #3!

Your PR introduces a lot of other changes though, and changes the whole repo structure; This makes it very hard to review it. Can you split it into several commits?

Also, some changes that I don't agree with:

  • hand-rolling query-string – I think it provides very nice defaults, is there a specific reason why you removed it?
  • With the GTFS-RT fields, did you switch from underscores to camel casing because gtfs-realtime-bindings exposes the fields this way? I really liked before that they directly match the GTFS-RT spec.
  • some stylistic changes which make Git diffs noisier, e.g. export function

@ForsakenHarmony
Copy link
Author

ForsakenHarmony commented Jun 26, 2021

hand-rolling query-string – I think it provides very nice defaults, is there a specific reason why you removed it?

to remove the dependency given it's only used in one place 🙃, I'll revert it when I get around to it

did you switch from underscores to camel casing because gtfs-realtime-bindings exposes the fields this way?

yes

some stylistic changes which make Git diffs noisier, e.g. export function

yes sorry, personal preferences (I consider default exports an anti-pattern), I'll remove them along with the revert above (again when and if I get around to it)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

fix protocol buffers parsing improve map view UX
2 participants