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

Add all routing parameters to debug UI #6370

Merged

Conversation

a-limyr
Copy link
Contributor

@a-limyr a-limyr commented Jan 9, 2025

Summary

This PR does one main thing, and also has a side-effect.

The main feature is to dynamically create UI for all arguments in the trip query schema.

The side-effect, but also a feature, is a redesign of the layout of the client.

Issue

Links to issue #6258.

The process for generating UI for all arguments works like this:
When the tool is built a code-gen script is run to create a json structure of the arguments and a trip query. This will make sure that both the trip query and the arguments are based on the current schema.

The json structure is then read by a React component (TripQueryArguments) that generates UI for setting arguments for the trip query. There is a file excluded-arguments.ts that is used to exclude any arguments from being generated. This file is to ensure the main input fields (in the search bar) don't have duplicate inputs.

The generated trip query file is the file used for performing queries in the tool and when opening the query in GraphiQL.

When it comes to the changes in layout there are three main changes:

  1. Left sidebar: contains itinerary results, filters (input arguments) and a view of the input variables in json format.
  2. Right sidebar: Refactored this to use React and not raw DOM manipulation. Added also a hide/show function.
  3. Top bar: Made bit more stable and contained, so that the main inputs do not overflow into the itinerary results.

…eft side of screen and a lot of other minor changes.
…de arguments logic, moved attribution control and added reset option for boolean arguments.
# Conflicts:
#	client/package-lock.json
#	client/package.json
#	client/src/components/MapView/LayerControl.tsx
#	client/src/components/SearchBar/SearchBar.tsx
#	client/src/static/query/tripQuery.tsx
…de arguments logic, moved attribution control and added reset option for boolean arguments.
@leonardehrenfried
Copy link
Member

I would love to see a screenshot.

@a-limyr
Copy link
Contributor Author

a-limyr commented Jan 15, 2025

Screenshot 2025-01-15 at 13 04 12
Screenshot 2025-01-15 at 13 04 30
Screenshot 2025-01-15 at 13 04 45
Hi, here are some screenshots.

Copy link

codecov bot commented Jan 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.77%. Comparing base (5ab75af) to head (131f301).
Report is 67 commits behind head on dev-2.x.

Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #6370      +/-   ##
=============================================
+ Coverage      69.73%   69.77%   +0.04%     
- Complexity     18023    18051      +28     
=============================================
  Files           2057     2060       +3     
  Lines          76978    77122     +144     
  Branches        7845     7856      +11     
=============================================
+ Hits           53678    53812     +134     
- Misses         20550    20556       +6     
- Partials        2750     2754       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@a-limyr a-limyr marked this pull request as ready for review January 23, 2025 08:16
@a-limyr a-limyr requested a review from a team as a code owner January 23, 2025 08:16
client/codegen-preprocess.ts Outdated Show resolved Hide resolved
@leonardehrenfried
Copy link
Member

From the screenshots it looks like this is no longer possible:

Screencast.From.2025-01-23.12-53-07.mp4

Am I right in thinking this?

@leonardehrenfried
Copy link
Member

If you're not fetching the schema file from a running OTP instance, do you still need the endpoint that @t2gran added to the Transmodel API?

@a-limyr
Copy link
Contributor Author

a-limyr commented Jan 23, 2025

From the screenshots it looks like this is no longer possible:

Screencast.From.2025-01-23.12-53-07.mp4
Am I right in thinking this?

I see now this disappeared in refactoring. The refactoring of the debug layer menu, was meant to move from raw DOM manipulation and to React. I missed this. Would appreciate if you could have a look at the debug layer menu and see if there are other issues.

It should be possible to recreate this function. I can have a look.

@a-limyr
Copy link
Contributor Author

a-limyr commented Jan 23, 2025

If you're not fetching the schema file from a running OTP instance, do you still need the endpoint that @t2gran added to the Transmodel API?

No, that was a misunderstanding from my part. The schema is now read twice. Once during build, to generate the trip query file used by the client for queries. And once via introspection at runtime, to get the actual schema for the running OTP instance, to generate the complete list of the arguments.

Just to be clear. The endpoint is not used/needed.

@testower
Copy link
Contributor

Just to be clear on why we both need to read the static schema on compile-time, AND do introspection in the runtime: The former is also needed to generate typescript types (and to generate the query code, which also needs to be present on compile-time for type-checking to work) - whereas the runtime introspection step is there to correctly generate the form components based on the actual schema running on the server.

@testower testower self-requested a review January 23, 2025 13:51
testower
testower previously approved these changes Jan 23, 2025
@a-limyr
Copy link
Contributor Author

a-limyr commented Jan 23, 2025

I have added the debug layer group toggle functionality requested.

@testower testower self-requested a review January 23, 2025 14:08
@leonardehrenfried leonardehrenfried changed the title Debug client dynamic arguments Add all routing parameters to debug UI Jan 23, 2025
@leonardehrenfried
Copy link
Member

Screencast.From.2025-01-23.17-44-37.mp4

Thanks it works beautifully!

Copy link
Member

@leonardehrenfried leonardehrenfried left a comment

Choose a reason for hiding this comment

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

image

We spoke about this in today's dev meeting and some people - including myself - had some requests for how the screen is laid out. However, everybody agreed that we don't want to prevent this great improvement from being merged in order to move elements around the screen.

Thanks for this great work!

@leonardehrenfried leonardehrenfried merged commit 1a86905 into opentripplanner:dev-2.x Jan 23, 2025
6 checks passed
t2gran pushed a commit that referenced this pull request Jan 23, 2025
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.

3 participants