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

feat: add complete metadata config example with xpaths #67

Merged
merged 3 commits into from
Sep 19, 2023

Conversation

Kate-Lyndegaard
Copy link
Contributor

ING-3963

@wetransformer wetransformer added lang-missing-cs Czech translation is not included in this PR lang-missing-de German translation is not included in this PR help-content Changes to the help content were done labels Aug 8, 2023
@Kate-Lyndegaard
Copy link
Contributor Author

@morch23mj How can I create a new line for the xpath values?

grafik

@morch23mj
Copy link
Member

morch23mj commented Aug 9, 2023

@morch23mj How can I create a new line for the xpath values?

That's possible through <br/> elements. I have pushed a commit containing those.

Copy link
Member

@morch23mj morch23mj left a comment

Choose a reason for hiding this comment

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

Do we need to update DE and CS documents too?

Copy link
Member

@JohannaOtt JohannaOtt left a comment

Choose a reason for hiding this comment

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

@Kate-Lyndegaard I think this list is very valuable. I just wonder if it could be a bit too much input for someone reading the documentation. Did you consider providing the list for download somewhere or the like and only link it from the documentation or put it on a separate page of the documentation so that advanced users can read it and less advances users are not overwhelmed?
Do we also need to add it to the German version?

@Kate-Lyndegaard
Copy link
Contributor Author

Do we need to update DE and CS documents too?

@morch23mj Yes, that would be good.

Did you consider providing the list for download somewhere or the like and only link it from the documentation or put it on a separate page of the documentation so that advanced users can read it and less advances users are not overwhelmed?

@JohannaOtt I simply updated the existing documentation. If we want to change it now to link to a document, that is fine with me. @morch23mj Where would I store such a document that I could then link to?

Do we also need to add it to the German version?

We can link the document on the German page, or is something else needed?

@JohannaOtt
Copy link
Member

Not sure if we also want to translate the descriptions and labels in the config. But IMO, users using the metadata config usually understand English, so I would tend to leave it as is.

@morch23mj
Copy link
Member

morch23mj commented Sep 5, 2023

@Kate-Lyndegaard @JohannaOtt I just checked and there is also the possibility of having a collapsable section (example here). Is that maybe easier? If so, I can try to work on it.

@Kate-Lyndegaard
Copy link
Contributor Author

@morch23mj Oooh, I like the collapsable section. That would be great here.

@Kate-Lyndegaard
Copy link
Contributor Author

@morch23mj Can this be merged, or should it wait for additional changes?

@morch23mj
Copy link
Member

morch23mj commented Sep 18, 2023

@Kate-Lyndegaard I think if it's not super urgent, first makes sense to add the changes related to the collapsible section.

@Kate-Lyndegaard
Copy link
Contributor Author

@morch23mj I tried to add the collapsible section, however I got the following:
grafik

@morch23mj
Copy link
Member

@Kate-Lyndegaard At least from the image, I see that </p> have no opening tag, and if it does, it should do after the <li> tag. If it's alright with you, I can also take that over. :)

@Kate-Lyndegaard
Copy link
Contributor Author

@morch23mj I tried to follow the directions in the link you posted. It would be great if this is a quick fix. I just don't like leaving large and useful PRs hanging around.

@wetransformer wetransformer removed lang-missing-cs Czech translation is not included in this PR lang-missing-de German translation is not included in this PR labels Sep 18, 2023
@morch23mj
Copy link
Member

morch23mj commented Sep 18, 2023

@Kate-Lyndegaard @JohannaOtt I've made the two sections collapsable, and documented how it'd done in the README.md file for future reference.

There are two example sections (both collapsable), one is minimal (the one used before) and the other one is very detailed. I thought it's a good idea to keep both? If the short one isn't needed anymore then it can be deleted before merging.

Screenshot 2023-09-18 at 16 35 39

Added same for other locales: de/cs.

@JohannaOtt
Copy link
Member

@morch23mj Only looked at the screenshot now. Is there an easy way to change the design of them to adapt it to our documentation. Otherwise, LGTM
@Kate-Lyndegaard We could consider creating a ticket to move all config examples etc to such sections so that the documentation parts get less long and users can look at them in case they want more detailed information.

@morch23mj
Copy link
Member

morch23mj commented Sep 19, 2023

@JohannaOtt Thanks for the feedback. :)

I have inspected what styles are applied there, and it seems it's the ones coming from our theme. In this case, it's alert-info that's applied which has this default blue. I have copied over a component from our docs next to it, which is an alert-info too, and the two match the same original style.

Yellow color is reserved for warning and red for danger (e.g. errors), and so on.

Screenshot 2023-09-19 at 12 15 23

If you still see that we could improve something here, your input is welcome!

@Kate-Lyndegaard
Copy link
Contributor Author

@morch23mj Thank you, @morch23mj !

@Kate-Lyndegaard Kate-Lyndegaard merged commit 6420941 into master Sep 19, 2023
2 checks passed
@Kate-Lyndegaard Kate-Lyndegaard deleted the complete-md-config branch September 19, 2023 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help-content Changes to the help content were done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants