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

OracleReportSanityChecker docs #225

Merged
merged 8 commits into from
Jun 29, 2023
Merged

Conversation

Psirex
Copy link
Contributor

@Psirex Psirex commented Jun 28, 2023

No description provided.

@Psirex Psirex requested a review from a team as a code owner June 28, 2023 12:01
Copy link
Member

@folkyatina folkyatina left a comment

Choose a reason for hiding this comment

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

Looks like reference format is really differs from other docs. Can we do it in one style, please?

docs/contracts/oracle-report-sanity-checker.md Outdated Show resolved Hide resolved
docs/contracts/oracle-report-sanity-checker.md Outdated Show resolved Hide resolved
@folkyatina folkyatina self-requested a review June 29, 2023 07:19
Copy link
Member

@folkyatina folkyatina left a comment

Choose a reason for hiding this comment

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

Much better, thank you. But have a couple more of a formatting requests, sorry.

We really need to do it automatically in the future.

docs/contracts/oracle-report-sanity-checker.md Outdated Show resolved Hide resolved
docs/contracts/oracle-report-sanity-checker.md Outdated Show resolved Hide resolved
Copy link
Contributor

@TheDZhon TheDZhon left a comment

Choose a reason for hiding this comment

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

Thanks to your efforts and work done, and with this PR, we have enshrined in docs a significant part of the word-of-mouth knowledge 👍

Please consider integrating the following changes if suitable: #226 🙏

@TheDZhon
Copy link
Contributor

TheDZhon commented Jun 29, 2023

Let's ship it 🚢

Thank you, @Psirex and @folkyatina 🍻

@TheDZhon TheDZhon merged commit 69f7564 into main Jun 29, 2023
@TheDZhon TheDZhon deleted the feat/oracle-report-sanity-checker branch June 29, 2023 20:29
@TheDZhon TheDZhon mentioned this pull request Jun 29, 2023
33 tasks
@arwer13
Copy link
Contributor

arwer13 commented Jun 30, 2023

The latest changes in OracleReportSanityChecker describe function arguments as a list in "Arguments" section.
But in others contracts it is "Parameters" and a table. Should we unify it as in other contracts? Sorry for noticing this so late :/

@folkyatina
Copy link
Member

folkyatina commented Jun 30, 2023

The latest changes in OracleReportSanityChecker describe function arguments as a list in "Arguments" section. But in others contracts it is "Parameters" and a table. Should we unify it as in other contracts? Sorry for noticing this so late :/

I'm not sure that it's so critical and I tend to use lists instead of tables too, because tables is so hard to format and have a type, which already included in the declaration part

@TheDZhon
Copy link
Contributor

Can't say that I have a strong preference here. Both arguments are sound 🤔

My general idea is that we would need a polishing iteration anyway, in the end, to unify the style again (cause we miss something else)

@Psirex
Copy link
Contributor Author

Psirex commented Jun 30, 2023

I agree with @folkyatina. Adding only two points from myself:

  • tables don't allow using multiline markdown markup inside cells. For example, lists and code blocs might be added only via JSX tags.
  • on mobile devices, tables with long content are often rendered with horizontal scrolls, what also less convenient than a regular list.

@arwer13
Copy link
Contributor

arwer13 commented Jun 30, 2023

Added "final clean-up" subtask in the master issue #217

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.

4 participants