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

DS-579 Header Height HTML Attribute #2537

Merged
merged 4 commits into from Sep 7, 2022

Conversation

MarcinMr
Copy link
Collaborator

@MarcinMr MarcinMr commented Sep 1, 2022

Jira

https://pegadigitalit.atlassian.net/browse/DS-579

Summary

A data-header-height attr was added to the <header />

Details

  • The height of the bolt-page-header component is added to the data-header-height attr
  • The data-header-height attr updates on resizing when the height of the header changes

How to test

Pull the branch. Check if data-header-height attr is added properly to the bolt-page-header component. Check if this attr updates when the component's height changes on window resize.

Release notes

data-header-height attribute is added to the bolt-page-header componenet.

@github-actions github-actions bot added the type: feature List this PR in the 'Features' section of the release notes. label Sep 1, 2022
…o feature/DS-579-Header-Height-HTML-Attribute
@colbytcook colbytcook requested a deployment to feature/DS-579-Header-Height-HTML-Attribute--5728ece--commit-preview September 1, 2022 14:07 Abandoned
@colbytcook colbytcook temporarily deployed to feature/DS-579-Header-Height-HTML-Attribute--branch-preview September 1, 2022 14:08 Inactive
@colbytcook colbytcook had a problem deploying to feature/DS-579-Header-Height-HTML-Attribute--branch-preview September 1, 2022 14:20 Failure
@colbytcook colbytcook had a problem deploying to feature/DS-579-Header-Height-HTML-Attribute--branch-preview September 6, 2022 13:24 Failure
@colbytcook colbytcook requested a deployment to feature/DS-579-Header-Height-HTML-Attribute--a421423--commit-preview September 7, 2022 14:28 In progress
@colbytcook colbytcook had a problem deploying to feature/DS-579-Header-Height-HTML-Attribute--branch-preview September 7, 2022 14:42 Failure
Copy link
Collaborator

@danielamorse danielamorse left a comment

Choose a reason for hiding this comment

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

Looks great! Approved.

I added the optimized-resize import. throttledResize worked because this dependency was already imported by some other components, but we should also import it here so that each package's dependencies are made explicit. Webpack will dedupe them.

When we start actually using this new data attribute, we may or may not want to switch to getBoundingClientRect() to get the height*, but it's too soon to say. For now clientHeight is a good choice.

*getBoundingClientRect() will return a fraction which can be used in a calculation with other fractions. If the header height is already rounded to an integer it may result in sub-pixel rounding issues, e.g. creating a 1px gap between the header and the sticky element we're trying to position.

@@ -2,6 +2,7 @@ import {
BoltPageHeaderNav,
BoltPageHeaderActionNav,
} from './page-header-nav.js';
import '@bolt/core-v3.x/utils/optimized-resize';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Added optimized-resize import. That's where throttledResize comes from.

@colbytcook colbytcook requested a deployment to hotfix/v5.7.3--83d8b19--commit-preview September 7, 2022 21:36 In progress
@danielamorse danielamorse merged commit 2a4c425 into master Sep 7, 2022
@danielamorse danielamorse deleted the feature/DS-579-Header-Height-HTML-Attribute branch September 7, 2022 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature List this PR in the 'Features' section of the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants