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 minor UI issues with default.md #2278

Merged
merged 12 commits into from
Apr 17, 2023
Merged

Conversation

lhw-1
Copy link
Contributor

@lhw-1 lhw-1 commented Apr 12, 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:

Addresses two of the issues related to _markbind/default.md brought up in #1866.

Specific improvements:

  • Change the sidebar name from "Template" to "Contents" in default.md
  • Remove generation of blank lines after the navbar in default.md

Anything you'd like to highlight/discuss:

Testing instructions:
Try markbind init or markbind init --convert on a pre-existing set of documents, and ensure that changes are reflected.

Proposed commit message: (wrap lines at 72 characters)
Fix minor UI issues with default.md


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 lhw-1 marked this pull request as ready for review April 12, 2023 09:42
@lhw-1 lhw-1 requested a review from a team April 12, 2023 09:50
@tlylt
Copy link
Contributor

tlylt commented Apr 16, 2023

@lhw-1 need resolve conflict

@lhw-1
Copy link
Contributor Author

lhw-1 commented Apr 16, 2023

Here is a summary of the changes made targeting "Remove generation of blank lines after the navbar in default.md".

In the current version of MarkBind, when the user calls markbind init --convert with a pre-existing set of documents, the section between the navbar and the footer in the generated default.md looks like this:

  ...
  </nav>
</div>



<footer>
  
<!-- Support MarkBind by including a link to us on your landing page! -->
<div class="text-center">
  <small>[Generated by {{MarkBind}}]</small>
</div>

</footer>

The issue brought up in #1866 is that there are unnecessary blank lines here that are generated during our convert process. After some investigation, the blank spaces are generated because we are currently using a nunjucks variable in our siteConvertLayout.njk, which dictates what the resulting default.md should look like:

  ...
  </nav>
</div>

{% set defaultFooter %}
<!-- Support MarkBind by including a link to us on your landing page! -->
<div class="text-center">
  <small>[Generated by {% raw %}{{MarkBind}}{% endraw %}]</small>
</div>
{% endset %}

<footer>
  {{ footer or defaultFooter }}
</footer>

In the case where the defaultFooter declared above is used, the blank lines before and after the {% set %} tags will remain in the file after the nunjucks variable is substituted into the <footer> tag below. This is why I directly moved it into the <footer> tag like this in the siteConvertLayout.njk:

  ...
  </nav>
</div>

<footer>
{% set defaultFooter %}
<!-- Support MarkBind by including a link to us on your landing page! -->
<div class="text-center">
  <small>[Generated by {% raw %}{{MarkBind}}{% endraw %}]</small>
</div>
{% endset %}
{{ footer or defaultFooter }}
</footer>

And this will result in the following default.md generated:

  ...
  </nav>
</div>

<footer>


<!-- Support MarkBind by including a link to us on your landing page! -->
<div class="text-center">
  <small>[Generated by {{MarkBind}}]</small>
</div>

</footer>

The blank lines between the navbar and the footer are now reduced to a single blank line (which is acceptable), but there are now new unnecessary blank lines introduced within the <footer> itself. These are leftover newline characters from after the nunjucks variables are rendered. Hence, I added "outer -" to the {% set %} tags to remove all blank lines before the {% set %} and after the {% endset %}. Here is what I tried with siteConvertLayout.njk:

  ...
  </nav>
</div>

<footer>
{%- set defaultFooter %}
<!-- Support MarkBind by including a link to us on your landing page! -->
<div class="text-center">
  <small>[Generated by {% raw %}{{MarkBind}}{% endraw %}]</small>
</div>
{% endset -%}
{{ footer or defaultFooter }}
</footer>

And the resulting default.md generated:

  ...
  </nav>
</div>

<footer>
<!-- Support MarkBind by including a link to us on your landing page! -->
<div class="text-center">
  <small>[Generated by {{MarkBind}}]</small>
</div>

</footer>

Since there is still one newline within the <footer> itself, I added "inner -" to the {% set %} tags to further remove all the newlines encased between the {% set %} and the {% endset %} tags. Here is what I tried with siteConvertLayout.njk:

  ...
  </nav>
</div>

<footer>
{%- set defaultFooter -%}
<!-- Support MarkBind by including a link to us on your landing page! -->
<div class="text-center">
  <small>[Generated by {% raw %}{{MarkBind}}{% endraw %}]</small>
</div>
{%- endset -%}
{{ footer or defaultFooter }}
</footer>

And the resulting default.md generated:

  ...
  </nav>
</div>

<footer>
<!-- Support MarkBind by including a link to us on your landing page! -->
<div class="text-center">
  <small>[Generated by {{MarkBind}}]</small>
</div>
</footer>

This is the behavior we would like to see, without unnecessary blank lines.


Unfortunately, because we remove the inner blank lines, this will affect custom footers that users may use. When I updated our functional tests, the footer for many of them were modified. For example, in the <footer> section of packages/cli/test/functional/test_site_convert/test_basic_convert/expected/Home.html, the code was modified from:

<footer>
  Custom footer.
</footer>

To:

<footer>Custom footer.
</footer>

In order to keep the behavior of custom <footer> consistent with previous behavior (which adds a newline), I modified Site/index.ts here to have a newline added when there is a pre-existing footer in the documents being converted.

Copy link
Contributor

@tlylt tlylt left a comment

Choose a reason for hiding this comment

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

LGTM

@tlylt tlylt added this to the v5.0.0 milestone Apr 16, 2023
@tlylt tlylt added the breakingChange 💥 Feature will behave significantly different, or is made obsolete label Apr 16, 2023
@tlylt
Copy link
Contributor

tlylt commented Apr 16, 2023

@lhw-1 possible to update the outdated content in here? https://markbind-master.netlify.app/devguide/bootcamp/exploremarkbind#modify-site-structure-and-configuration

Should have done so in your previous PR updating the template.

Copy link
Contributor

@tlylt tlylt left a comment

Choose a reason for hiding this comment

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

Can you have another look at packages/core/src/Site/siteConvertLayout.njk to ensure it's up-to-date?

@lhw-1
Copy link
Contributor Author

lhw-1 commented Apr 16, 2023

Can you have another look at packages/core/src/Site/siteConvertLayout.njk to ensure it's up-to-date?

Thanks for the catch! There does seem to be inconsistencies between default.md and siteConvertLayout.njk. I've just updated it in 09e7264.

On another note, I wonder if we can work on a further refactor to have siteConvertLayout.njk dictate the behavior for the default.md generated during both init and init --convert (the former is dictated by the default.md in the templates directory, while the latter is dictated by siteConvertLayout.njk). Given that default values can be specified in siteConvertLayout.njk, it might be a better idea to consolidate them into a single file. What do you think?

@tlylt
Copy link
Contributor

tlylt commented Apr 16, 2023

On another note, I wonder if we can work on a further refactor to have siteConvertLayout.njk dictate the behavior for the default.md generated during both init and init --convert (the former is dictated by the default.md in the templates directory, while the latter is dictated by siteConvertLayout.njk). Given that default values can be specified in siteConvertLayout.njk, it might be a better idea to consolidate them into a single file. What do you think?

Good suggestion and yes can explore. I wonder if there will be a case where the two (default and convert) will deviate further in the future. From the perspective of the functionality, I think it feels more natural to have "default.md" dictate "siteConvertLayout.njk"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breakingChange 💥 Feature will behave significantly different, or is made obsolete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants