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

🐛 FIX: Boostrap -> Bootstrap in code/docs #44

Merged
merged 4 commits into from
Oct 12, 2020
Merged

Conversation

choldgraf
Copy link
Member

@choldgraf choldgraf commented Sep 30, 2020

I noticed in the docs that there seemed to be a typo for the bootstrap config, it was called boostrap instead of bootstrap.

But then when I looked at the code, I saw that the config was also boostrap and the docs were technically correct.

So this PR changes boostrap to bootstrap in the config and docs, assuming that this was a typo. Is that right @chrisjsewell ?

NOTE: when this changes we'll also need to do the same thing in Jupyter Book...I dunno how we missed this one.

This makes me think we may actually need to run a deprecation cycle so that users aren't jarred by the change. @chrisjsewell what do you think?

@welcome
Copy link

welcome bot commented Sep 30, 2020

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out EBP's Code of Conduct and our Contributing Guide, as this will greatly help the review process.

Welcome to the EBP community! 🎉

@choldgraf choldgraf changed the title docs typo 🐛 FIX: Boostrap -> Bootstrap in code/docs Sep 30, 2020
@choldgraf
Copy link
Member Author

choldgraf commented Oct 9, 2020

@chrisjsewell I'm not sure what to do here re: deprecation cycle. I'll go with whatever you prefer. Lemme know if you want me to add a deprecation note to this 👍

@choldgraf
Copy link
Member Author

OK I went ahead and added a Sphinx warning if add_boostrap_css is there. This is ready for review/merge whenever @chrisjsewell

LOGGER.warning(
"`add_boostrap_css` will be deprecated. Please use `add_bootstrap_css`."
)
app.config.add_bootstrap_css = app.config.panels.add_boostrap_css
Copy link
Member

Choose a reason for hiding this comment

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

want happens here though if someone sets panels_add_bootstrap_css=False?
won't this override that?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah that's a good point, I forgot it defaults to True. I'm not sure how to check for whether a configuration value has been provided by a user or not :-/

Copy link
Member

@chrisjsewell chrisjsewell Oct 12, 2020

Choose a reason for hiding this comment

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

perhaps add_boostrap_css and panels_add_bootstrap_css should be set to None initially, to check if someone actually sets a value on them.

Then:

  1. warn if add_boostrap_css is not None
  2. fail if both are not None
  3. If one is not None, take that value (check it is a bool)
  4. default to True

Copy link
Member Author

Choose a reason for hiding this comment

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

well I guess if they both default to True, then can't we assume that if either of them is False, then they should both be False? So the check would become:

    if not app.config.panels_add_boostrap_css:
        LOGGER.warning(
            "`panels_add_boostrap_css` will be deprecated. Please use `panels_add_bootstrap_css`."
        )
        app.config.panels_add_bootstrap_css = app.config.panels_add_boostrap_css

Copy link
Member

Choose a reason for hiding this comment

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

Well my way is more rigorous, you decide 🤷

@chrisjsewell
Copy link
Member

crazy I/we didn't notice this before 🤦

@choldgraf
Copy link
Member Author

I know - I only noticed it after several weeks of having it called boostrap in my own docs too. I'll make the tests pass and ping you but need to deal with a crying baby for a bit

@choldgraf
Copy link
Member Author

choldgraf commented Oct 12, 2020

OK crying baby was not crying for long 😅

new default is None and the over-writing / CSS excluding only happens if we have an explicit value set 👍

@choldgraf
Copy link
Member Author

while I was at it I updated the documentation to make the config key a separate code block so it's easier to find.

@chrisjsewell
Copy link
Member

chrisjsewell commented Oct 12, 2020

After merging, you should try running https://github.com/executablebooks/sphinx-panels/blob/master/git_rebase_theme_branches.sh

This cycles through all the theme branches and rebases them on to master (i.e. incorporating the latest commit(s)), which then triggers the RTD build for each.
Literally the only difference between these branches should be the default theme name on this line:

theme_name = os.environ.get("HTML_THEME", "sphinx_rtd_theme")

This step should actually be added to the dev instructions

@choldgraf
Copy link
Member Author

choldgraf commented Oct 12, 2020

Happy to give that a shot. Alternatively if you wanna add a section to the dev-docs with those instructions / what they do / etc, I am happy to follow those steps on my own to make sure that they work and give feedback in the devdocs PR?

@choldgraf choldgraf merged commit 285c89b into master Oct 12, 2020
@welcome
Copy link

welcome bot commented Oct 12, 2020

Congrats on your first merged pull request in this project! 🎉
congrats

Thank you for contributing, we are very proud of you! ❤️

@choldgraf choldgraf deleted the choldgraf-patch-1 branch October 12, 2020 17:38
@chrisjsewell
Copy link
Member

chrisjsewell commented Oct 12, 2020

Nah just give it a go and I'll do the instructions after: the instruction is literally just open a terminal and run ./git_rebase_theme_branches.sh, that should do it 🤞

sphinx-panels recipe has also just been merged into conda, so once that builds I can add the badge for that at the same time

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.

2 participants