Skip to content
This repository has been archived by the owner on Aug 13, 2023. It is now read-only.

Adds bottom nav border in high contrast mode #4475

Merged
merged 6 commits into from
May 10, 2021

Conversation

mukama
Copy link
Contributor

@mukama mukama commented May 6, 2021

Resolves #4291

Overall change:
This PR adds a bottom border to the navigation component. This is visible only in high contract mode.

Before
image

After
image

Code changes:

  • Adds an after pseudo-element of with no height and a transparent background. This is only visible in high contrast mode.

  • I have assigned myself to this PR and the corresponding issues
  • Automated jest tests added (for new features) or updated (for existing features)
  • This PR requires manual testing

@mukama mukama self-assigned this May 6, 2021
@github-actions
Copy link

github-actions bot commented May 6, 2021

Checkout your storybook preview here http://psammead-preview.tools.bbc.co.uk/4475

@mukama mukama changed the title Adds bottom border in high contrast mode Adds bottom nav border in high contrast mode May 6, 2021
Copy link
Contributor

@HarryVerhoef HarryVerhoef left a comment

Choose a reason for hiding this comment

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

Nice one! Think this just needs the psammead-navigation package to be bumped then the CI will pass 👍

Copy link
Contributor

@andrewscfc andrewscfc left a comment

Choose a reason for hiding this comment

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

Looks good, just a suggestion on the version number

package.json Outdated
@@ -86,7 +86,7 @@
"@bbc/psammead-media-indicator": "6.1.0",
"@bbc/psammead-media-player": "5.1.0",
"@bbc/psammead-most-read": "6.0.28",
"@bbc/psammead-navigation": "9.1.1",
"@bbc/psammead-navigation": "9.1.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"@bbc/psammead-navigation": "9.1.2",
"@bbc/psammead-navigation": "9.2.0",

I think this should be a minor package bump as component is changing behaviour (not just an internal bug fix) see https://semver.org/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestion @andrewscfc. Thanks for the review

@@ -3,6 +3,7 @@
<!-- prettier-ignore -->
| Version | Description |
|---------|-------------|
| 9.1.2 | [PR#4475](https://github.com/bbc/psammead/pull/4475) Adds bottom nav border in high contrast mode |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| 9.1.2 | [PR#4475](https://github.com/bbc/psammead/pull/4475) Adds bottom nav border in high contrast mode |
| 9.2.0 | [PR#4475](https://github.com/bbc/psammead/pull/4475) Adds bottom nav border in high contrast mode |

@@ -1,6 +1,6 @@
{
"name": "@bbc/psammead-navigation",
"version": "9.1.1",
"version": "9.1.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"version": "9.1.2",
"version": "9.2.0",

Copy link
Contributor

@andrewscfc andrewscfc left a comment

Choose a reason for hiding this comment

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

Thanks @mukama

@greenc05
Copy link
Contributor

Great, thanks @mukama Please could you confirm if this will be implemented across all page types? I will just check this is as expected in Edge on Windows in High Contrast Mode... I assume those screen shots are from Firefox with the colour preferences changed?

@greenc05
Copy link
Contributor

I can confirm this looks as expected on the preview url in Edge and IE11 on Windows 10 in a custom high contrast mode.

@mukama
Copy link
Contributor Author

mukama commented May 10, 2021

Great, thanks @mukama Please could you confirm if this will be implemented across all page types? I will just check this is as expected in Edge on Windows in High Contrast Mode... I assume those screen shots are from Firefox with the colour preferences changed?

That is correct! The screenshots are from Firefox with custom colour preferences turned on

@mukama mukama merged commit 7eb0863 into latest May 10, 2021
@joshcoventry joshcoventry deleted the fix-navigation-banner-border-issues branch September 9, 2021 11:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Navigation Banner transparent bottom border missing when user changes colours
4 participants