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(theme): support disabling aside globally #1925

Merged
merged 5 commits into from
Feb 12, 2023
Merged

Conversation

dominikcz
Copy link
Contributor

fix #1915

@brc-dd brc-dd changed the title support disabling aside in themeConfig feat(theme): support disabling aside globally Feb 12, 2023
@brc-dd brc-dd merged commit dd0c4c6 into vuejs:main Feb 12, 2023
@dominikcz
Copy link
Contributor Author

I'm not sure if that last commit with logic makes it better. For me original logic, which I just extended was more legible as it clearly stated that aside is turned off in case of:

  • home page
  • explicitly setting frontmatter.value.aside to false (not true, null or undefined)
  • explicitly setting theme.value.aside to false (not true, null or undefined).

Now for some reason you treat frontmatter.value.aside in a different way than theme.value.aside while they do behave in similar way: can be null, undefined, true or false. Don't get me wrong, both approaches will work correctly, for me original writing was just clearer as far as developer's intentions are considered.

There is also one issue that bothers me a bit: in documentation I stated that default value is true. It's not really the case s default value is undefined, but figured that it's easier for end-user to think about it as true because it behaves like true. Hope it's OK for you.

Anyway thank you for merging :)

@brc-dd
Copy link
Member

brc-dd commented Feb 12, 2023

The logic for the change was that frontmatter should always override themeConfig. Your logic would not have worked if someone wants to "enable" aside selectively for a page via frontmatter (when they have disabled aside globally).

There is also one issue that bothers me a bit: in documentation I stated that default value is true. It's not really the case s default value is undefined, but figured that it's easier for end-user to think about it as true because it behaves like true. Hope it's OK for you.

Yeah that's fine. For some of the other things in the config, we are doing the same.

@dominikcz
Copy link
Contributor Author

dominikcz commented Feb 12, 2023

The logic for the change was that frontmatter should always override themeConfig. Your logic would not have worked if someone wants to "enable" aside selectively for a page via frontmatter (when they have disabled aside globally).

I see, wasn't aware of that. Thank you!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support disabling aside in themeConfig
2 participants