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.

Sign up for GitHub

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

Jump to bottom

Switch to documentation tables based on xcore #438

Merged
merged 22 commits into from
Aug 24, 2024
Merged

Switch to documentation tables based on xcore #438

merged 22 commits into from
Aug 24, 2024

Conversation

sgrossberndt
Copy link
Contributor

No description provided.

@sgrossberndt
Copy link
Contributor Author

Ok, we're finally in a state now to easily deploy, render and compare the state of the current documentation rendering and the state of the new xcore rendering.

Also there is a new rudimentary starting page https://vdvde.github.io/OJP/ which shows the available documentation renderings based on branches.

The new documentation rendering found some issues, most in SIRI but one also is in OJP:

*** Create report for domain: name="ojp"
*** Item without type, path: "choice[]/siri:TrainRef"
*** Item without type, path: "choice[]/siri:CompoundTrainRef"

sgrossberndt
sgrossberndt commented Mar 28, 2024
View reviewed changes
Copy link
Contributor Author

@sgrossberndt sgrossberndt left a comment

Choose a reason for hiding this comment

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

There are multiple issues with the current state of the new documentation rendering which have to be solved before merging this and thus making it our new documentation:

This is only a first check, there are probably more issues to be spotted which can be done by others as well now.

docs/generate_tables/custom-ojp-perxsd.xml
<!-- Report type specific rules -->
<reportTypes/>
<!--
<domains>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this commented section really relevant or should it be removed?

docs/generate_tables/xco-reporter.xqm Outdated Show resolved Hide resolved
docs/generate_tables/xco-html.xqm Outdated
[contains($href, 'enum-dict')]
return
<li>{
<span class="monospace">{$prefix}</span>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this prefix really necessary in our case? I'd opt to remove it and restore the standard bullets to the list

docs/generate_tables/xco-html.xqm Show resolved Hide resolved
@trurlurl
Copy link
Contributor

@sgrossberndt - Very nice that it now (almost) works!

I've spotted a few more problems, see below.

As for the missing sections 1.11 to 1.24 in the table of contents - I think this is by design and not inappropriate, since it concerns nested, unnamed types. There are similarly missing sections 12.32 ff. They all concern SIRI groups, and the tables (i.e., those not figuring in the table of contents) are replicated in both the OJP and the SIRI file. I don't know whether the replication in the OJP file somehow makes sense, e.g., for the links to work? So my question would rather be: could/should they be removed from the OJP file? But they don't do much harm, in my view.

Further small problems:

  1. Number formatting looks a bit arbitrary at first glance (but isn't). The impression would be cleaner if always the same formatting were applied. For easening the transfer to MS Word spaces should be avoided.
    grafik

  2. In the case of <xs:choice minoccurs="0"> it should rather say, instead of "contains one of the following...": "contains none or one of the following...".
    grafik

  3. Many of the links in https://vdvde.github.io/OJP/ do not work - is this because merging has not been done yet?

Are we (aka @sgrossberndt) able to do the suggested improvements on our own?

@sgrossberndt
Copy link
Contributor Author

@sgrossberndt - Very nice that it now (almost) works!

yes 🎉

I've spotted a few more problems, see below.

As for the missing sections 1.11 to 1.24 in the table of contents - I think this is by design and not inappropriate, since it concerns nested, unnamed types. There are similarly missing sections 12.32 ff. They all concern SIRI groups, and the tables (i.e., those not figuring in the table of contents) are replicated in both the OJP and the SIRI file. I don't know whether the replication in the OJP file somehow makes sense, e.g., for the links to work? So my question would rather be: could/should they be removed from the OJP file? But they don't do much harm, in my view.

I'd like to have someone explain to me why these unnamed complex types are part of ojp.html. I don't see a reason for it. If they were only part of siri.html, I wouldn't mind.

Further small problems:

1. Number formatting looks a bit arbitrary at first glance (but isn't). The impression would be cleaner if always the same formatting were applied. For easening the transfer to MS Word spaces should be avoided.

I agree.

2. In the case of _<xs:choice minoccurs="0">_ it should rather say, instead of "contains _one_ of the following...": "contains _none or one_ of the following...".

I agree.

3. Many of the links in https://vdvde.github.io/OJP/ do not work - is this because merging has not been done yet?

I only did a first version which still contained links to release/1.0 which did not have a rendered documentation back then. I excluded those links now in 246645e. Once 2.0 has been released it will be visible as release/2.0 and we could add a link to main again.

Are we (aka @sgrossberndt) able to do the suggested improvements on our own?

I am not able to do those suggested improvements.

@ue71603
Copy link
Contributor

ue71603 commented Apr 30, 2024

Hans-Jürgen will have a look at it until next week.

@sgrossberndt sgrossberndt force-pushed the feature/documentation-tables-xcore branch from 3937431 to 1769af1 Compare June 19, 2024 14:39
@sgrossberndt
Wrap CDATA section in Javascript comments
1a6ff97 
The CDATA section in the Javascript tag leads to
`Uncaught SyntaxError: expected expression, got '<'`
if not wrapped in a Javascript comment
@sgrossberndt
Move CSS and Javascript to separate files
b05f6c1 
* Move CSS definitions to the CSS file
* Move Javascript code to a file
* Include at end of body instead of head, but only in the report where it is needed
@sgrossberndt
Correct Javascript include
4e9305a
@sgrossberndt
Copy link
Contributor Author

Please review this pull request by comparing the state of the current documentation and the state of the new xcore documentation rendered using this pull request.

@ue71603
Copy link
Contributor

ue71603 commented Jun 20, 2024

@trurlurl Will you do the first comparison?

@trurlurl
Copy link
Contributor

trurlurl commented Jul 4, 2024

For me, everything looks fine now!

ue71603
ue71603 previously approved these changes Jul 4, 2024
View reviewed changes
trurlurl
trurlurl previously approved these changes Jul 4, 2024
View reviewed changes
@sgrossberndt
Copy link
Contributor Author

I propose to add a space before the (↔ so the definitions break to the next line (as done manually in 2.8 in this example (while 2.7 does not have the space):
Screenshot 2024-07-05 at 14-40-26 OJP - Open API for di
This improves readability a lot as otherwise the description column is very small

@sgrossberndt
Copy link
Contributor Author

I propose to add a space before the (↔ so the definitions break to the next line (as done manually in 2.8 in this example (while 2.7 does not have the space): Screenshot 2024-07-05 at 14-40-26 OJP - Open API for di This improves readability a lot as otherwise the description column is very small

@skinkie The issue of a line break here is the only reason why it has not been merged yet:
https://vdvde.github.io/OJP/feature/documentation-tables-xcore/documentation-tables/ojp.html#element_ojp__OJPLineInformationDelivery

@trurlurl
Copy link
Contributor

@sgrossberndt I agree, this is a good idea how to "nudge" column widths and to improve readability and layout.
I think inserting a space (or newline) in the following line in file xco.html.xqm should do that: ! ('(', <em>↔ {string()}</em>, ')')
Would you give it a try?

@sgrossberndt sgrossberndt dismissed stale reviews from trurlurl and ue71603 via 7044562 August 23, 2024 19:25
@sgrossberndt
Copy link
Contributor Author

Looks good to me. Please review and re-vote

@sgrossberndt sgrossberndt merged commit 765a3dd into develop Aug 24, 2024
2 checks passed
@sgrossberndt sgrossberndt deleted the feature/documentation-tables-xcore branch August 24, 2024 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trurlurl trurlurl trurlurl approved these changes

@skinkie skinkie skinkie approved these changes

@ue71603 ue71603 Awaiting requested review from ue71603

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sgrossberndt @trurlurl @ue71603 @skinkie @hrennau