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

Clean up default init template #2165

Merged
merged 12 commits into from
Mar 12, 2023
Merged

Clean up default init template #2165

merged 12 commits into from
Mar 12, 2023

Conversation

lhw-1
Copy link
Contributor

@lhw-1 lhw-1 commented Feb 13, 2023

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:

Cleans up the default template for markbind init, consisting of minor changes in wording, etc.

As this PR is not tied to a specific issue, any suggestions on improving the current template (in terms of pure cosmetics without changing the functionalities) are welcome.

Anything you'd like to highlight/discuss:

Testing instructions:

Proposed commit message: (wrap lines at 72 characters)
Clean up default init template


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

@lhw-1
Copy link
Contributor Author

lhw-1 commented Feb 14, 2023

Update in b7c8cbd:

The placeholder text in the topicx.md files were changed to be:

# Topic XX

> This is a placeholder page - more content to be added.

Some of the whitespaces in 404.md were removed to be standardized with the rest of the .md files. Additionally, the text was changed from "File not found ..." to "Page not found ...".

@lhw-1 lhw-1 requested a review from a team February 15, 2023 08:30
@itsyme
Copy link
Contributor

itsyme commented Feb 15, 2023

Hi @lhw-1! Thanks for the PR! Just a small nit in the home page under "Heading 3". I noticed that for the third panel: "Expanded panel", the panel is meant to be expanded on load, however, it seems to be missing the expanded property in index.md. Perhaps you could include that to make the panel expand on load as intended!

@lhw-1
Copy link
Contributor Author

lhw-1 commented Feb 17, 2023

Thanks for looking through the changes @itsyme!

Just a small nit in the home page under "Heading 3". I noticed that for the third panel: "Expanded panel", the panel is meant to be expanded on load, however, it seems to be missing the expanded property in index.md. Perhaps you could include that to make the panel expand on load as intended!

This is the current output for index.md under "Heading 3":

Screenshot from 2023-02-17 14-56-36

Essentially, the minimized panel at the top is set to change its header back to "Expanded panel" once it is no longer minimized (by clicking on it at least once):

Screenshot from 2023-02-17 14-56-53

Additionally, the expanded panel at the bottom will become a minimized panel as well after clicking the "X" button:

Screenshot from 2023-02-17 14-57-14

So I believe that the heading "Expanded panel" refers not to having the panel already expanded on load, but to differentiate between a minimized and an expanded panel - I don't think including the property for this bottom panel is a necessity, since it could potentially further obfuscate the meaning of "expanded" for new users (if it already isn't doing so...).

Regardless, what you've pointed out is still a good example to showcase for panels (especially to differentiate between these two meanings of "expanded"), so it may be worth including another panel at the bottom that showcases this property specifically!

On that note, it might also be worth looking into improving the template to be more comprehensive to a new user, instead of having filler texts. With reference to the above example, some brief explanation of the features could be more handy than the standard Lorem ipsum 😄 This is probably out of scope for this PR, though.

@tlylt
Copy link
Contributor

tlylt commented Feb 28, 2023

Just a small nit in the home page under "Heading 3". I noticed that for the third panel: "Expanded panel", the panel is meant to be expanded on load, however, it seems to be missing the expanded property in index.md. Perhaps you could include that to make the panel expand on load as intended!

Essentially, the minimized panel at the top is set to change its header back to "Expanded panel" once it is no longer minimized (by clicking on it at least once):

Screenshot from 2023-02-17 14-56-53

Additionally, the expanded panel at the bottom will become a minimized panel as well after clicking the "X" button:

Screenshot from 2023-02-17 14-57-14

So I believe that the heading "Expanded panel" refers not to having the panel already expanded on load, but to differentiate between a minimized and an expanded panel - I don't think including the property for this bottom panel is a necessity, since it could potentially further obfuscate the meaning of "expanded" for new users (if it already isn't doing so...).

Regardless, what you've pointed out is still a good example to showcase for panels (especially to differentiate between these two meanings of "expanded"), so it may be worth including another panel at the bottom that showcases this property specifically!

If you add the expanded property in, does it look better/make the example clearer? I think this is a good point - I re-read the conversation above at least twice to understand what this is all about, it's definitely not the clearest example that we can give. Would be great if we can modify this part slightly to make the example clear.

FYI the deployed output of the standard template can be viewed here.

@lhw-1
Copy link
Contributor Author

lhw-1 commented Mar 1, 2023

If you add the expanded property in, does it look better/make the example clearer? I think this is a good point - I re-read the conversation above at least twice to understand what this is all about, it's definitely not the clearest example that we can give. Would be great if we can modify this part slightly to make the example clear.

Right now, the wording we are using in the template for "Expanded panel" vs "Minimized panel" is referring to the panel header, and not the panel body itself, which is what the expanded property is for. So I think that we can add in a third panel to showcase the expanded panel using the expanded property, and also change the wording for the two panels to "Expanded panel header" and "Minimized panel header". How does that sound?

@tlylt
Copy link
Contributor

tlylt commented Mar 1, 2023

If you add the expanded property in, does it look better/make the example clearer? I think this is a good point - I re-read the conversation above at least twice to understand what this is all about, it's definitely not the clearest example that we can give. Would be great if we can modify this part slightly to make the example clear.

Right now, the wording we are using in the template for "Expanded panel" vs "Minimized panel" is referring to the panel header, and not the panel body itself, which is what the expanded property is for. So I think that we can add in a third panel to showcase the expanded panel using the expanded property, and also change the wording for the two panels to "Expanded panel header" and "Minimized panel header". How does that sound?

Sure let's try that 🚀

@lhw-1
Copy link
Contributor Author

lhw-1 commented Mar 11, 2023

As per suggestions above, I've included a new panel in 4ebf484:

image

Which makes more sense to me 😄

@lhw-1
Copy link
Contributor Author

lhw-1 commented Mar 11, 2023

Also, given that this PR has been up for quite some time & there has not been more suggestions, I think it's fine to consider this PR as complete.

(Requesting for approving reviews!)

@tlylt tlylt added this to the v4.2.0 milestone Mar 11, 2023
@tlylt tlylt merged commit 161eb2f into MarkBind:master Mar 12, 2023
@lhw-1 lhw-1 modified the milestones: v4.2.0, v5.0.0 Apr 3, 2023
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.

3 participants