Update Popover with center alignments and Justify.Fit #295
Conversation
|
Oh, I guess I should probably add testcases for the new enum values. Regardless, this is ready for review while I do that. |
|
Size Change: +1.31 kB (0%) Total Size: 519 kB
ℹ️ View Unchanged
|
|
Alright, that should be full test coverage for the new values now |
|
Note: The
The popover will then correct its position the next time the browser window is resized or if the mutation observer is triggered. Changes to the |
scripts/expectToBePx.ts
Outdated
| import diff from 'jest-diff'; | ||
|
|
||
| expect.extend({ | ||
| toBePx(received, expected) { |
There was a problem hiding this comment.
I mean, I guess so? I'd just as much update the tests to have them match so we don't have to maintain this.
There was a problem hiding this comment.
My first pass updated all the tests with a regex replace, but then it turned out that not all of the internal functions seemed to be consistent with whether it used a number or a string with px. I suppose a helper function in the test file might be easier to maintain, though.
There was a problem hiding this comment.
I feel semi strongly to not take on this debt, but I'm curious about @DesignerDave's thoughts.
There was a problem hiding this comment.
function checkPixelValue(actual: string | number, expected: number) {
if (typeof actual === 'string') {
expect(actual).toBe(`${expected}px`);
} else {
expect(actual).toBe(expected);
}
}it's a pretty easy regex find/replace to just use this instead, so that's probably worth the small tradeoff in having very slightly worse diffs and error messages.
There was a problem hiding this comment.
Updated with the helper function
|
@hswolff addressed your comments |
|
@DesignerDave I just pushed up a commit that fixes the issue when changing to/from |
| break; | ||
| } | ||
| // Constructs the transform origin for any given pair of alignment / justification | ||
| function getTransformOrigin({ align, justify }: TransformOriginArgs): string { |
| } | ||
|
|
||
| switch (justification) { |
There was a problem hiding this comment.
Thank you for removing these crazy switch statements
|
Right, and that's still effectively the case if you do align=left and justify=middle or whatever, although since I removed the |
|
Alright, I think I was misunderstanding. With the latest I'm still finding issues when switching between alignments:
All of these were triggered from switching to, and then from a center alignment. I'm having trouble reproducing the padding issue in particular, but I can pretty consistently reproduce the alignment issues. |
|
I switched the storybook popover to have |
|
Whoops, copy + paste error. Try this:
That should get you to at least the last screenshot I sent. Doing the following will also result in a strange alignment:
|
|
Oh, there it is! Just reproduced it. |
|
Alright, so I had it re-render when changing to or from |
|
Thanks for the thorough debugging @DesignerDave! Just pushed up an update that should cover those additional cases. I'm not sure if there's a good way to write an automated test for those types of cases without a browser environment, but I also doubt that there's a realistic usecase for switching between a vertically-oriented alignment and a horizontal one, so we should be fine I think. |
|
@jetpacmonkey Those fixes seem to be working well! Thanks for the update. Little more QA on components currently using popover: Tooltip – Everything else is looking good now it seems! Awesome stuff. |
|
I'm back from the worst-timed vacation in history! I don't think I realized that Tooltip was extending |
|
@DesignerDave I think that should be everything. I left |
|
Hey @jetpacmonkey, sorry for the delayed response. I was testing out tooltip, and it looks like align left/right with justify fit doesn't work: Fit should work for those alignments, right? Otherwise this seems ready to go! |
|
Yeah, that's actually working the same as it does in |
✍️ Proposed changes
As part of the work for adding a
<Select>component, Popover needed some additional functionality. This PR adds support foralign={Align.CenterHorizontal},align={Align.CenterVertical}, andjustify={Justify.Fit}. I also removed theJustificationenum, as it's become redundant withAlign, and refactored a bunch of internals inpositionUtilsto derive values directly fromAlignandJustify, without usingJustificationas a go-between.Some of the changes led to test failures in the
positionUtilsunit tests because of a change from30to'30px'(for example), so instead of changing them all over, I wrote a custom matcher that accepts either of the two formats. This feels like a better test (to me, at least) since it should generally avoid false positives on changes that don't actually impact behavior.🎟 Jira ticket: PD-271
🛠 Types of changes
✅ Checklist
yarn changesetand documented my changes💬 Further comments
This is, unfortunately, a breaking change, since
justificationwas being passed to the function-as-a-child pattern. I've also included a fix for Tooltip, which was relying on that behavior. I would assume that we probably don't have any external consumers relying on Popover, so that should be sufficient.