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

Configurable styling (with an interest in VIA) #21

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

Borketh
Copy link
Contributor

@Borketh Borketh commented May 21, 2024

This is just a start, and doesn't include any content changes (24h time and different ARDP handling), only CSS.
Crucially, this does not actually change anything except the colour of the VIA h1 to their yellow colour. It's supposed to make the timetables look like this testing timetable I tweaked manually:
image
However, it does not seem to be properly applying the via-special-css class into the table elements themselves.
Obviously, this is an inherently inefficient way to try this in the first place. Ideally, the style would be configurable by switching between entire stylesheets and/or processes entirely. I'm kind of stuck as I have limited knowledge of Jinja. How would you suggest this be done? Relevant documentation would be appreciated.

For reference, this is the VIA style guide. Oddly enough, their own website uses a different shade of yellow...

@Borketh Borketh changed the title Draft PR for adding VIA-esque styling DO NOT MERGE Draft PR for adding VIA-esque styling May 21, 2024
@Borketh
Copy link
Contributor Author

Borketh commented May 21, 2024

My intention here is that CSS snippets be dynamically loadable loadable instead of front loading everything and deciding what style gets used with umpteen different tags. This would also mean completely unnecessary CSS could be left out of a given timetable (for example, the Hudson Bay timetable will never need to know how to style the MBTA logo, but that CSS is still added right now).

@neroden
Copy link
Owner

neroden commented May 23, 2024

Yeah, the current system for assembling CSS is a bit brute-force. There are several considerations. I've been trying to do the semantic separation properly in HTML vs CSS (so that the stylesheet can be overridden) and that took me a while to get even approximately right.

At this point you really should just be able to apply a cascading stylesheet which is prioritized over the built-in one (as a user stylesheet with your client browser). I haven't tested it -- I meant to. If it doesn't work that's a bug.

Part of the problem is that you seem to have some confusion regarding good CSS selector practice. CSS selectors are... weird and counterintuitive. I suggest reviewing their bizarre syntax and semantics. I think you want to add "via-special-css" to the tag for the table, or better yet the

tag for the entire page, and then catch descendants of it in your stylesheet, rather than adding the tag to every single element, which is what you're trying to do.

The elements are mostly tagged being functionally (type of cell, etc.), though there are some leftovers (row and column number tracking) copied from the old PANDAS code.

I was planning to provide an option for an external rather than embedded stylesheet but then the HTML isn't self-contained and aaaargh this has its own problems when I go to push the file through the PDF converter.

The icon & logo measurement stuff is incredibly tedious, since it's really not stuff which belongs in CSS but that's how things work with modern HTML/CSS (I tried several other approaches to get the measurements into the HTML and they all had fatal flaws of one sort or another). This is actually one of the major obstructions to a clean style vs. substance separation.

I could probably collect the relevant CSS fragments for connecting services icons the way I collect the relevant connecting services for producing the HTML list -- note that I also currently ship all the connecting services icon files, relevant or not, and the same collection-list approach is probably desirable. This could probably be done relatively elegantly in the connecting services module. Have a function in the module, to which you pass a list of connecting services back into the module and have it return the CSS. Have another function to which you pass as list of connecting services and have it return the list of icon files.

Don't put the if-then logic into the RPA logo file. I'm trying to separate the HTML which describes an image and how to present it from the logic which decides whether or not to include it. Makes it easier if they change the frickin' logo or if I start having other sponsor logos to choose from. (I would have a whole subdirectory of these, like the connecting services files... if there were more than one. But it's not worth overengineering.)

Anyway those are just some incomplete design notes. I wish I had the time and energy to work on this myself right now.

@Borketh
Copy link
Contributor Author

Borketh commented May 24, 2024

I think you want to add "via-special-css" to the tag for the table, or better yet the tag for the entire page, and then catch descendants of it in your stylesheet, rather than adding the tag to every single element, which is what you're trying to do.

Oh right, that's a thing. Can you tell frontend isn't my forte?

Have a function in the module, to which you pass a list of connecting services back into the module and have it return the CSS. Have another function to which you pass as list of connecting services and have it return the list of icon files.

Makes sense. Will you do that or do I?

Don't put the if-then logic into the RPA logo file.

Good call, that was an earlier change I made before I fully understood the loading. Should the rpa_logo be more generic in future so that we can accommodate other logos (such as Transport Action Canada).

Anyway those are just some incomplete design notes. I wish I had the time and energy to work on this myself right now.

This is very helpful though, thank you for taking the time to explain this! I'll try to find the time to work on it after this round of school deliverables.

@neroden
Copy link
Owner

neroden commented May 27, 2024

I think you want to add "via-special-css" to the tag for the table, or better yet the tag for the entire page, and then catch descendants of it in your stylesheet, rather than adding the tag to every single element, which is what you're trying to do.

Oh right, that's a thing. Can you tell frontend isn't my forte?

CSS is very... unusual... and I had to spend a lot of time relearning CSS to get any of this done. Not my forte either really!

Have a function in the module, to which you pass a list of connecting services back into the module and have it return the CSS. Have another function to which you pass as list of connecting services and have it return the list of icon files.

Makes sense. Will you do that or do I?

Depends who gets to it first, right? If you get to it before I do I'll probably tweak it, of course.

Good call, that was an earlier change I made before I fully understood the loading. Should the rpa_logo be more generic in future so that we can accommodate other logos (such as Transport Action Canada).

Yeah, it should probably be something like "sponsor" with a selection of sponsors. Good thought

Borketh and others added 6 commits June 3, 2024 14:58
this cuts down on the act of passing way too many kwargs and calling agency_singleton for every method call

it also makes it easier to get new config options in functions that use the class without refactoring the function signature to explicitly ask for it
now for via content changes
@Borketh
Copy link
Contributor Author

Borketh commented Jun 5, 2024

@neroden I'd like to hear your thoughts on how I would implement agency specific content inclusion, such as times being in 12/24hr format, the text "Train #" above tsns, and extra station information such as station titles.

@neroden
Copy link
Owner

neroden commented Jun 12, 2024

Ahhh, yes. This stuff.

So here's an underlying issue: my division into agencies with agency-specific code was designed around reading different GTFS files, because each agency has its own quirks of GTFS format. Which is necessary.

What we're looking at here is a different distinction -- just as I can present VIA timetables with Amtrak style, we could present Amtrak timetables with VIA style. (Or timetables in another house style for New York by Rail... I was considering discussing that with the guy who publishes that.)

I think what we arguably need is a --style parameter to the main run of timetable.py. While some of the differences end up in the stylesheet, as you've pointed out, others don't, they end up in content.

I'm not entirely sure how to orthogonally implement the style options, but I think it needs to be a separate parameter passed to the main code, so that choices like the time format can be switched off of that in the main codebase.

But... maybe it shouldn't be a command-line parameter. After all, style is what the spec files are all about. Maybe it should be a parameter in the .toml file. I like that, actually.

@Borketh
Copy link
Contributor Author

Borketh commented Jun 17, 2024

I think both would work. We could have the .toml file say what the default style should be but be able to override that with a --style parameter.

So how should this be implemented? I'm thinking something like parallel classes to Agency. AgencyStyle, AmtrakStyle, ViaStyle, etc. Should that be a subclass or member of Agency, or not related at all? My preference would be class member referencing the relevant style class. The agency_special_css and possibly other future methods of handling the agency-specific css could go there as well.

@neroden
Copy link
Owner

neroden commented Jun 17, 2024 via email

@Borketh
Copy link
Contributor Author

Borketh commented Aug 10, 2024

@neroden got a chance to work on this again. What are your thoughts on this initial API draft?

@Borketh Borketh changed the title DO NOT MERGE Draft PR for adding VIA-esque styling Draft PR for adding configurable styling (with an interest in VIA) Aug 10, 2024
@neroden
Copy link
Owner

neroden commented Aug 12, 2024 via email

@Borketh
Copy link
Contributor Author

Borketh commented Aug 14, 2024

You noticed how long the day string code was? I had SO MANY tweak requests for that before all the Amtrak fans were satisfied.

Yeah lol, it's a lot. Like I said in the comment I left, there's probably a slightly better way to do it but it's not worth the effort right now lol.

There's too much which needs to be done on Covid safety right now, which is life and death, and I have a bunch of personal stuff going on too.

Good luck! No rush to merge anything. I'm still working on integrating it into the rest of the codebase, and I don't want to accidentally remove functionality when it's ready.

also fixes bug with amtrak json_stations where the directory is not created properly
fixes a bug where column options would show up as empty after being asked for twice (csv, html) leading to weird bugs in output timetables like "Read Down" incorrectly being applied and daystrings not being added to multi-day trips
@Borketh Borketh marked this pull request as ready for review August 24, 2024 09:54
@Borketh
Copy link
Contributor Author

Borketh commented Aug 24, 2024

Here's the difference a simple flag now makes (--style via-tac) to the Canadian's timetable. The only issue is now the default style overrides the owner of the route (lmao) because that's tied to agency-special-css. That should probably be retied to the agency classes themselves rather than being inserted by CSS with that class.

image
image

@Borketh
Copy link
Contributor Author

Borketh commented Aug 24, 2024

One thing I'd like to mention: all amtrak timetables seem to be broken and I don't know how to diagnose the issue. No trains are present in the GTFS by the time it gets to look for the ones it needs for a given timetable. Maybe it's a code issue, maybe it's a reference date issue, maybe the feed itself is faulty - I have no idea. Just a heads up.

@Borketh
Copy link
Contributor Author

Borketh commented Aug 24, 2024

This is going to be a big review, and I look forward to the epic roast of some of the stuff I did. I suggest going per-commit because taking it in as the sum of all commits looks like a nightmare. There's no rush to get this merged, as I can generate what I need for TAC from my fork. Good luck with the things you're dealing with right now!

@Borketh Borketh changed the title Draft PR for adding configurable styling (with an interest in VIA) Configurable styling (with an interest in VIA) Aug 24, 2024
@neroden
Copy link
Owner

neroden commented Sep 19, 2024 via email

@Borketh
Copy link
Contributor Author

Borketh commented Sep 25, 2024

That's great to hear! I don't have much time to work on this now that university is back in session. I would like to get the amtrak issues fixed before this is merged but I'm not sure anything I did in this PR is the problem. Needs further investigation...

@neroden
Copy link
Owner

neroden commented Nov 14, 2024 via email

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