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 temp styles not applying to navbars in header files #892

Merged
merged 2 commits into from
Jun 28, 2019

Conversation

alyip98
Copy link
Contributor

@alyip98 alyip98 commented Jun 26, 2019

What is the purpose of this pull request? (put "X" next to an item, remove the rest)
• [x] Bug fix

Alleviates #889

What is the rationale for this request?
Previously, #664 added temporary styles to <navbar> to prevent them from displaying as a list while Vue loads. However, after the layout update, the insertTemporaryStyles was called before insertHeaderFile, which kept the temporary styles from being applied to <navbar> in the header file.

What changes did you make? (Give an overview)
This PR swaps the order of the two calls, such that <navbar> in the header file will also get the temporary styles applied.

Provide some example code that this change will affect:
ezgif-1-423f7783287c

Is there anything you'd like reviewers to focus on?

Testing instructions:
netlify preview

Proposed commit message: (wrap lines at 72 characters)
Previously, a fix was implemented for reducing FOUC by applying
temporary styles for and . A later commit broke this
fix by moving the header and footer files to a layout, which caused
the contents of those files to be missed by the temporary style fix.

This commit fixes this by inserting the temporary styles after
the header and footer files are inserted in the page, such that
the temporary styles are applied correctly to the relevant tags.

src/Page.js Outdated
.then(result => this.insertHeaderFile(result))
.then(result => this.insertTemporaryStyles(result))
Copy link
Contributor

@openorclose openorclose Jun 26, 2019

Choose a reason for hiding this comment

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

Should put this after insertFooterFile? So that the header and footer are logically grouped together.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good suggestion, since there is a chance that navbars or dropdown menus are placed in a footer by the author.

@alyip98
Copy link
Contributor Author

alyip98 commented Jun 26, 2019

A more permanent solution can be explored with pre-rendering Vue components in the future. But for now this minor fix helps alleviate the FOUC issue

@damithc
Copy link
Contributor

damithc commented Jun 26, 2019

I was wondering why the navbar was causing a ugly FOUC when it was minimized a few versions ago :-p

@Chng-Zhi-Xuan
Copy link
Contributor

A more permanent solution can be explored with pre-rendering Vue components in the future. But for now this minor fix helps alleviate the FOUC issue.

Might also want to revisit the original plan of displaying a spinner until the content + vue components have finished loading.

@damithc
Copy link
Contributor

damithc commented Jun 26, 2019

Might also want to revisit the original plan of displaying a spinner until the content + vue components have finished loading.

@marvinchin tried that. We eventually decided against it because it gave a 'slow site' feeling to the user.

Previously, a fix was implemented for reducing FOUC by applying
temporary styles for <navbar> and <dropdown>. A later commit broke this
fix by moving the header and footer files to a layout, which caused
the contents of those files to be missed by the temporary style fix.

This commit fixes this by inserting the temporary styles after
the header and footer files are inserted in the page, such that
the temporary styles are applied correctly to the relevant tags.
@Chng-Zhi-Xuan Chng-Zhi-Xuan changed the title Fix bug that prevented temp styles from applying to layout navbars Fix temp styles not applying to navbars in header files Jun 28, 2019
@Chng-Zhi-Xuan Chng-Zhi-Xuan merged commit cafcb05 into MarkBind:master Jun 28, 2019
@Chng-Zhi-Xuan Chng-Zhi-Xuan added this to the v2.5.2 milestone Jun 28, 2019
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.

4 participants