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

Docs for feature flag #5043

Merged
merged 10 commits into from
Jan 29, 2019
Merged

Docs for feature flag #5043

merged 10 commits into from
Jan 29, 2019

Conversation

dojutsu-user
Copy link
Member

Fixes #5041

@dojutsu-user
Copy link
Member Author

I request to please suggest imorovements for english.

Copy link
Member

@RichardLitt RichardLitt left a comment

Choose a reason for hiding this comment

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

Small language changes.

docs/faq.rst Outdated Show resolved Hide resolved
docs/faq.rst Outdated Show resolved Hide resolved
Copy link
Member

@stsewd stsewd left a comment

Choose a reason for hiding this comment

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

Please follow our guidelines for contributing to documentation https://docs.readthedocs.io/en/latest/docs.html

@dojutsu-user
Copy link
Member Author

@stsewd
Thank you.
I have pushed some changes and tried to follow the docs pointed by you.

docs/faq.rst Outdated Show resolved Hide resolved
docs/faq.rst Outdated Show resolved Hide resolved
.. _github: https://github.com/rtfd/readthedocs.org

Available Flags
---------------
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@stsewd
I didn't get it.
Do you want to render the whole list (with description) with rst role? or just the description? or any other thing?

Copy link
Member

Choose a reason for hiding this comment

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

List and description

Copy link
Member Author

Choose a reason for hiding this comment

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

@stsewd
I have tried it and i suppose that the function for "role" is working fine. However I am not able to get the output at the docs.
Can you please help me in this. I am unable to find the mistakes.
The output I'm getting is

featureflags:

Copy link
Member Author

Choose a reason for hiding this comment

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

@stsewd
It was a wonderful idea. I managed to implement it successfully and now the flags with their description are rendering as desired automatically.
Here is the image of the feature flags docs page - image

Copy link
Contributor

Choose a reason for hiding this comment

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

I love adding features like this to our docs! 💯

I would suggest a change here: instead of using a role to put all of the different feature flags in a single block, this should mimic what the django setting role does.

Also, if you were to implement a feature like this normally, you'd use a directive, not a role. I do think role would be preferred here if you were to match how we are using the django setting role. It allows us to put more commentary around the output.

Copy link
Member Author

@dojutsu-user dojutsu-user Jan 11, 2019

Choose a reason for hiding this comment

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

@agjohnson
I am a little confused.
This is what I'm getting now.

  • In the rst file, the use of the role should be like this.
    ``FEATURE_1``: :featureflags:`FEATURE_1`
    ``FEATURE_2``: :featureflags:`FEATURE_2`
    ``FEATURE_3``: :featureflags:`FEATURE_3`
    
    which will render the feature flags with its description as nodes.Text.

But doing this, we will lost the automatic rendering of the docs.
Also, the docs will look like this - image

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want to fully automate things, there might be some feature flags we don't want documented or don't need documented.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for the explanation.
I have followed your advice and changed the pattern of the role similar to djangosettings role.

@humitos
Copy link
Member

humitos commented Jan 10, 2019

Can you upload a screenshot so we can see how it will looks like? Thanks!

@dojutsu-user
Copy link
Member Author

@humitos
Yes, of course.
Here is the link to the image.

But the look of it may change as per the comment by @agjohnson above.

@humitos humitos mentioned this pull request Jan 14, 2019
@dojutsu-user
Copy link
Member Author

I have pushed the changes.
Here is how the docs page looks

screencapture-file-home-pikachu-desktop-rtd-readthedocs-org-docs-_build-html-guides-feature-flags-html-2019-01-18-16_13_29

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

Looks good. 👍

@ericholscher ericholscher merged commit 481497d into readthedocs:master Jan 29, 2019
@dojutsu-user dojutsu-user deleted the docs-for-feature-flags branch January 30, 2019 10:11
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.

6 participants