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

Implements fetch for readme file of glues #143

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

judibo
Copy link
Contributor

@judibo judibo commented Jul 27, 2022

Implements fetch for readme file of glues to solve the problem on issue #111 and dynamic renders the readme file at the documentation page.

Here is a print to check how it´s being rendered at the doc locally x readme at github for the herbs2gql glue.
image

I´m sending only 1 glue for now, so we can check how it will work on Production. If everything works fine, as it is running locally, I´ll implement on the other glues.

@jhomarolo
Copy link
Contributor

It is under conflict


<ReadMeDoc docURL='https://raw.githubusercontent.com/herbsjs/herbs2gql/master/README.md'/>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be inside a env file

Copy link
Contributor

@juliadibo juliadibo Jul 28, 2022

Choose a reason for hiding this comment

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

I wasn´t able to put it inside the md file, but I put the base URL inside the env file to import it at the react component and oly pass the endpoint at the md file.


return (
<>
<ReactMarkdown children={markdown} />
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if url is unavailable or broken?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point!
Just implemented a catch with an Error message, asking to go to github doc instead.

@jhomarolo
Copy link
Contributor

It is under conflict

@jhomarolo
Copy link
Contributor

Also, can you change all the glues?

@jhomarolo jhomarolo requested a review from dalssoft July 28, 2022 14:40
@judibo
Copy link
Contributor Author

judibo commented Jul 28, 2022

Hi @jhomarolo,
just updated with the other glues.

sidebar_label: Error to Text
slug: /glues/suma2text
hide_title: true
Copy link
Member

Choose a reason for hiding this comment

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

Why not create an .md generator? we could have all this information inside 1 file in json format, so the generator generates everything before deploying

Another suggestion is, why not read all repositories so auto-creates these files? it looks simple as all files have a similar structure

}, [])

return (
<>
Copy link
Member

Choose a reason for hiding this comment

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

this indentation is correct? It looks so long

@dalssoft
Copy link
Member

dalssoft commented Aug 7, 2022

First, I wanted to say that this PR is very important for the future maintenance of the Herbs documentation. Thank you @judibo !

However, as it is today, we have a problem: we replaced the local docs with the readme.md files from the glue repositories. These readme.md files are not up to date, nor structured for the site's documentation (ex: header structure, links, etc). Therefore, accepting this PR as it is today would be a step backwards in the quality of the documentation presented.
As a first step we should fix these files (readme.md files on each glue repo) to be able to accept this PR.

Ideally, we should update the repos with a structure that is not dependent on the readme.md file, like the structure suggested in issue #145.

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.

5 participants