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

Remove sticky-top class from side navigations #827

Merged
merged 1 commit into from
Apr 11, 2019

Conversation

marvinchin
Copy link
Contributor

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [x] Bug fix

Fix #818.

What is the rationale for this request?

The side navigations have unnecessarily high z-index, which causes it to be layered on top of other components.

What changes did you make? (Give an overview)

Remove the sticky-top bootstrap class which adds the high z-index, and use other css classes to retain the same positioning without increasing z-index.

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

Testing instructions:
Build the TE3201 website with this branch, and navigate to http://127.0.0.1:8080/se-book-adapted/chapters/refactoring.html#refactoring. Use the searchbar. The page navigation should not appear above the search results.

Screenshot 2019-04-10 at 12 19 32 PM

Screenshot 2019-04-10 at 12 22 10 PM

Proposed commit message: (wrap lines at 72 characters)

Remove sticky-top class from side navigations

The sticky-top class increases the z-index of the side navigations to
1030. The high z-index is not necessary for the side navigations, and
has caused it to be layered on top of the dropdowns of other components.

Let's remove the sticky-top class from the side navigations, and use
other css classes to achieve the same positioning without the high
z-index.

The sticky-top class increases the z-index of the side navigations to
1030. The high z-index is not necessary for the side navigations, and
has caused it to be layered on top of the dropdowns of other components.

Let's remove the sticky-top class from the side navigations, and use
other css classes to achieve the same positioning without the high
z-index.
@Chng-Zhi-Xuan
Copy link
Contributor

Chng-Zhi-Xuan commented Apr 10, 2019

Are you planning on reverting the changes to dropdown lists in vue-strap?

If so, it should be included in this PR (updated vue-strap.min.js).

@marvinchin
Copy link
Contributor Author

Yes, I'm intending to make those changes!

Re-building and updating vue-strap is done in our release procedure. Hence I feel like updating it should be done during release for consistency rather than in this PR (since there seems to be 2 logical parts to this - making changes to the vue-strap repo, and then updating the vue-strap dependency in the markbind repo).

What do you think about it?

@Chng-Zhi-Xuan
Copy link
Contributor

Chng-Zhi-Xuan commented Apr 10, 2019

As the changes were minimal for this PR, we can do the updating of vue-strap dependency on the next release.

For bigger PRs that cover features or bug fixes. Usually we require the updated vue-strap.min.js file within the PR in order for any feature / tests to work properly.

It is more for the reviewer's convenience, but not necessary for this PR :P

@marvinchin
Copy link
Contributor Author

Understood, thanks for the clarification! 🙂

Copy link
Contributor

@Chng-Zhi-Xuan Chng-Zhi-Xuan left a comment

Choose a reason for hiding this comment

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

Tested with the updated vue-strap.min.js file from MarkBind/vue-strap#104. Works fine as it should 👍

@yamgent yamgent added this to the v2.1.2 milestone Apr 10, 2019
@yamgent yamgent merged commit f3144f0 into MarkBind:master Apr 11, 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.

PageNav appears above search results
3 participants