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: allows header style overriding #38

Merged
merged 2 commits into from
Oct 14, 2024
Merged

Conversation

winterdouglas
Copy link
Contributor

Description

This change allows the Header styles to be fully overriden from the outside, including the internally calculated dimensions for the left, center and right containers.

Motivation and Context

It's a common approach to let the consumer override the styles from the outside by placing the style prop last, so that the users of the component can do whatever they want with the internal containers, even to override the inner width and minWidth that the component calculates. Besides that, it opens some other interesting options, like freely customizing the headerStyle with flex options to play along with its containers.

As a practical example imagine the need for the title container to occupy most of the horizontal space of the header, rather than 1/3 as it is now, before this change, setting a width or minWidth wouldn't have any effect, as the internal calculation will always prevail by precedence, same for flex options, as they are ignored with fixed widths.

How Has This Been Tested?

I have tested these changes in my own app, where I was able to override the styles by setting the width and minWidth.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have followed the guidelines in the README.md file.
  • I have updated the documentation as necessary.
  • My changes generate no new warnings.

Screenshots

Before

image

After

image

The code for the results above:

headerLeftStyle={{
  backgroundColor: 'red',
  minWidth: undefined,
  width: undefined,
}}
headerCenterStyle={{
  backgroundColor: 'green',
  minWidth: undefined,
  width: undefined,
}}
headerRightStyle={{
  backgroundColor: 'blue',
  minWidth: undefined,
  width: undefined,
}}

Additional Notes (Optional)

I consider this as a breaking change because people might have set custom styles on those containers that were being ignored until now, and will from now on be properly respected.

By unsetting the width and minWidth it may come with a tradeoff of poor header container alignments, like the title not being centered for uneven edges, but it's up to developer to adjust that if it's a deliberate decision to override those (like in my case). I do a custom calculation of the side container dimensions based on the larger edge width, that way it ensures that when the containers are uneven it will always make sure that the title container has the same distance to both edges and therefore the title can be center aligned if needed. This could be an interesting future improvement for the package, even more if you plan to support left aligned (android) or expanded center containers.

const [leftDimensions, onLeftLayout] = useComponentDimensions();
const [rightDimensions, onRightLayout] = useComponentDimensions();

const largerEdgeWidth = leftAligned
    ? undefined
    : Math.max(leftDimensions.width, rightDimensions.width);

// then on the side components style I do the following to guarantee that the title is always properly aligned.
minWidth: largerEdgeWidth

@e-younan
Copy link
Member

Hey @winterdouglas, thanks for opening this PR! I was thinking of improving the API of the Header component since it is definitely very rough around the edges. This is a step in the right direction.

@e-younan e-younan merged commit fc8ffc5 into codeherence:main Oct 14, 2024
3 checks passed
@e-younan
Copy link
Member

I released this under version 0.13.0. Once I have some more time, I'll look into improving the API!

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.

2 participants