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(stark-ui): update some component styling to fix bugs #1171

Merged

Conversation

carlo-nomes
Copy link
Collaborator

  • fix footer overflow
  • fix footer hovering above bottom of screen
  • removed unused redundant DOM from app.component.html (showcase / starter)

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #1169

What is the new behavior?

Footer sticks to bottom.

Does this PR introduce a breaking change?

[x] Yes
[ ] No
  • stark-app-sidenav-content should not be applied to a wrapper element (div) anymore. Replace it with a ng-container.
    This ensures the footer is always at the bottom. (see Starter for an example)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.445% when pulling 764ee3b on cnomes:fix/viewport-issues into 50e9bbe on NationalBankBelgium:master.

@coveralls
Copy link

coveralls commented Mar 5, 2019

Coverage Status

Coverage increased (+0.005%) to 93.451% when pulling 01257d9 on cnomes:fix/viewport-issues into 5a9df97 on NationalBankBelgium:master.

Copy link
Collaborator

@christophercr christophercr left a comment

Choose a reason for hiding this comment

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

LGTM... except for the fact that on IE the pages are empty now :p
ie empty pages

@carlo-nomes carlo-nomes force-pushed the fix/viewport-issues branch from 764ee3b to 5b980f0 Compare March 6, 2019 12:02
@carlo-nomes
Copy link
Collaborator Author

@christophercr I updated the PR. I also needed to change some CSS of the table-of-contents component

Copy link
Collaborator

@christophercr christophercr left a comment

Choose a reason for hiding this comment

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

Now although the issue on IE is solved, as I showed you there are 2 issues:

  1. The Table of Contents doesn't react anymore to the scrolling event to activate the topic in which you are.
    Before:
    old-table-of-contents-ok
    Now:
    issue-table-of-contents

  2. The scrollbar of the page (Getting Started) is not always clickable... apparently the Table of Contents overlaps in front of it.

@carlo-nomes carlo-nomes force-pushed the fix/viewport-issues branch from 5b980f0 to 53aeddb Compare March 6, 2019 14:23
@carlo-nomes
Copy link
Collaborator Author

@christophercr I updated the PR and added #1172 😄

@christophercr
Copy link
Collaborator

Now Travis is failing because of this error on the unit tests of the Showcase:

src/app/shared/components/table-of-contents/table-of-contents.component.spec.ts(42,13): error TS2339: Property 'scrollToAnchor' does not exist on type 'TableOfContentsComponent'.

@carlo-nomes
Copy link
Collaborator Author

@christophercr I updated the PR 😄

Copy link
Collaborator

@christophercr christophercr left a comment

Choose a reason for hiding this comment

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

LGTM

As discussed, the regression caused in the Table of Contents will be tackled in #1172 which has a lower priority that the issue solved in this PR ;)

carlo-nomes and others added 2 commits March 7, 2019 17:32
  - fix footer overflow
  - fix footer hovering above bottom of screen
  - removed unused redundant DOM from `app.component.html` (showcase / starter)

ISSUES CLOSED: NationalBankBelgium#1169

BREAKING CHANGES:

  - `stark-app-sidenav-content` should not be applied to a wrapper element (`div`) anymore. Replace it with a `ng-container`.
  This ensures the footer is always at the bottom. (see Starter for an example)

  - the css selector for `.stark-main-container` has changed and can now be applied directly to the `main` element in app.component.html.
  (see Starter for an example)
@carlo-nomes carlo-nomes force-pushed the fix/viewport-issues branch from 13e77cf to 01257d9 Compare March 7, 2019 16:33
@SuperITMan SuperITMan merged commit 14e432d into NationalBankBelgium:master Mar 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants