feat: add Popover component #373

micgro42 · 2021-02-11T17:13:42Z

This will be implemented across multiple subtasks of T273041.

github-actions · 2021-02-11T17:14:15Z

https://119bc6d25deb9bf2eb5d77a16ade033ac6931053--wmde-wikit.netlify.app

Ladsgroup · 2021-02-12T03:06:47Z

It is small enough to be easily reviewed and have my blessings. If UX looks good, this can go in? (I know this is not mergable atm)

bereket-WMDE

its a good baseline to continue the rest of the subtasks.
Its looks good. maybe a very very nitpicky suggestion: should we rename this.isShownData to this.isDataShown ? unless there's a reason I'm missing.

ofc, the failing tests need to be fixed. then can be approved and merged.

micgro42 · 2021-02-15T11:06:23Z

Its looks good. maybe a very very nitpicky suggestion: should we rename this.isShownData to this.isDataShown ? unless there's a reason I'm missing.

Yeah, or .isContentShown would be an even better name

bereket-WMDE · 2021-02-15T11:30:01Z

Its looks good. maybe a very very nitpicky suggestion: should we rename this.isShownData to this.isDataShown ? unless there's a reason I'm missing.

Yeah, or .isContentShown would be an even better name

I like that one

sai-san · 2021-02-26T17:38:43Z

Lots of progress here! 💯

Let me suggest the following Storybook changes hoping you can tell me if you agree this would be an improvement or not. Maybe, instead of the iconOnly buttons, we could use normal buttons with text that provides some extra indications, such as:

In the Basic page, the button label could say "Popover trigger"
In the All page, the button labels could indicate the position of the popover (like in this example).
It would also be awesome if, in that same page, the popovers were placed in an orderly fashion beyond the fold, so not so much scrolling is necessary.

I could try to do this myself in a different PR, since it doesn't impact the component. What do you think?

sai-san · 2021-02-26T17:42:04Z

Unfortunately, many of the popovers look broken in Safari:

Top-right and bottom-right are the only two that render correctly.

micgro42 · 2021-02-26T18:03:02Z

In the Basic page, the button label could say "Popover trigger"

Easily done. Will do it tomorrow.

In the All page, the button labels could indicate the position of the popover (like in this example).

Can do, but note that we would keep showing all the Popovers by default, to allow for visual regression tests to verify their position and appearance.

It would also be awesome if, in that same page, the popovers were placed in an orderly fashion beyond the fold, so not so much scrolling is necessary.

I agree, but that becomes tricky together with the above requirement of showing all the Popovers right from the start. Though we could probably reduce the distance between two components a bit, that should already help. And maybe change the order of the Popover-positions a bit, that should also help.

Unfortunately, many of the popovers look broken in Safari:

Yeah, we will have to look into fixing that. My suspicion is that our postcss is not translating things like inset-block-end: ... correctly into bottom: ...

micgro42 · 2021-02-26T18:04:08Z

In the mid-term, we may want to look into separating the storybook for visual regression tests from the storybook for public documentation.

And ideally have those visual regression tests done with all supported browsers, not just with Chrome.

sai-san · 2021-03-01T08:57:23Z

Can do, but note that we would keep showing all the Popovers by default, to allow for visual regression tests to verify their position and appearance.

Yes, showing the popovers by default is perfect, thanks for covering for that! At the moment, all the popovers disappear when any of the buttons in the All page is clicked, so I thought that using the button labels to indicate direction could be nice for orientation.

I agree, but that becomes tricky together with the above requirement of showing all the Popovers right from the start. Though we could probably reduce the distance between two components a bit, that should already help. And maybe change the order of the Popover-positions a bit, that should also help.

The All page looks so much better now! Thanks 🙏

micgro42 · 2021-03-01T08:59:14Z

@SaiSan-WMDE I adjusted storybook a bit. What do you think?

Safari is broken because of a problem with our PostCSS setup. I'll made a dedicated ticket for that: T276028.
It probably should go into its own pull request as it isn't really related to this PR, it is just the most obvious here.
But I think the checkbox might be broken in Safari as well? (not sure)

micgro42 · 2021-03-01T09:16:52Z

At the moment, all the popovers disappear when any of the buttons in the All page is clicked, so I thought that using the button labels to indicate direction could be nice for orientation.

Gotcha, will fix!

sai-san · 2021-03-01T12:55:26Z

It probably should go into its own pull request as it isn't really related to this PR, it is just the most obvious here.
But I think the checkbox might be broken in Safari as well? (not sure)

You're probably right 😕 I'll do an overall review in Safari to collect all the issues. I could document them in a Phab ticket in case that's useful.

micgro42 · 2021-03-01T14:28:38Z

It probably should go into its own pull request as it isn't really related to this PR, it is just the most obvious here.
But I think the checkbox might be broken in Safari as well? (not sure)

You're probably right confused I'll do an overall review in Safari to collect all the issues. I could document them in a Phab ticket in case that's useful.

Yes, if you find issues other than this and in the Checkbox, then another Phabricator ticket would be a good idea

micgro42 · 2021-03-01T15:26:55Z

Mh, changing those button labels revealed some more positioning problems. Will look into them today.

The scss use directive has to be added to vue.config.js because it has to be in front of all other rules. Co-authored-by: bereket-WMDE <71328938+bereket-WMDE@users.noreply.github.com>

Easiest would be to manually define two explicit rtl and ltr blocks to workaround transforms not being handled by postcss-logical, but that doesn't work because we're prefixing _everything_ with .wikit :/ So we need to figure out how to not rely on translateX and horizontally moved origins of transformation.

micgro42 · 2021-03-01T18:51:11Z

This should now fix most of the rtl positioning issue, except one, the horizontally centered position. That one needs more thinking.

translateX isn't adjusted by postcss-logical, therefore we have to make use of flexbox also somewhat affecting its absolute positioned children.

micgro42 · 2021-03-02T08:58:49Z

@SaiSan-WMDE This should now be finally ready to properly review :)

sai-san · 2021-03-02T09:30:57Z

The All page looks quite confusing now:

Why is the style of the buttons not matching normal neutral?
Probably those h2 should have a 100% width, so the buttons go back to be aligned in columns of 4.
Seeing RTL makes me think that probably we should rename all "right" and "left" props to "end" and "start" respectively. Otherwise, it's really confusing. What do you think?

micgro42 · 2021-03-02T09:39:05Z

The All page looks quite confusing now:
* Why is the style of the buttons not matching normal neutral?

Yikes, the color was only for testing purposes to see where the middle of the button is exactly! Will fix that!
The higher height was necessary, to ensure that the buttons are actually aligned to the middle, because previously they were sometimes aligned to the top or the button and it worked just because the small height of the button matched with the small height of the Popover.

* Probably those h2 should have a 100% width, so the buttons go back to be aligned in columns of 4.

Not sure what you mean, but can do.

* Seeing RTL makes me think that probably we should rename all "right" and "left" props to "end" and "start" respectively. Otherwise, it's really confusing. What do you think?

so top-start, top, top-end, end-top, end-center, end-bottom, bottom-end, bottom-center, bottom-start, start-bottom, start, start-top? 🤔

remove debugging color and make headline be on its own line.

This accounts for the Popover actually switching sides between ltr and rtl.

micgro42 · 2021-03-02T10:56:52Z

So, now it should be better. Let me know what you think, @SaiSan-WMDE 😊

sai-san · 2021-03-02T11:00:36Z

Thanks for all the changes! 🙏

so top-start, top, top-end, end-top, end-center, end-bottom, bottom-end, bottom-center, bottom-start, start-bottom, start, start-top? 🤔

Position will never stop being somewhat confusing, but maybe now the indicator is more aligned with the implementation, and it communicates the actual RTL position properly, unlike before:

sai-san · 2021-03-02T12:00:11Z

Safari issues will still need to be tackled (in a separate PR, as discussed), and the responsive behavior of popovers should be discussed, but I'd say at this point this is ready to be merged?

micgro42 · 2021-03-02T12:08:38Z

Safari issues will still need to be tackled (in a separate PR, as discussed), and the responsive behavior of popovers should be discussed, but I'd say at this point this is ready to be merged?

Wait, the Safari-Issues should be fixed. Do they still exist?

sai-san · 2021-03-02T13:38:33Z

Wait, the Safari-Issues should be fixed. Do they still exist?

I'm afraid they do, but only in the Basic page:

micgro42 · 2021-03-02T13:46:09Z

Wait, the Safari-Issues should be fixed. Do they still exist?

I'm afraid they do, but only in the Basic page:

Oh right. I have that fixed in a minute.

micgro42 · 2021-03-02T14:08:23Z

Wait, the Safari-Issues should be fixed. Do they still exist?

I'm afraid they do, but only in the Basic page:

Oh right. I have that fixed in a minute.

Should be fixed now :)

* First draft of the popover tokens * Update Popover.json * Update Popover.json * Update Popover.json * Update tokens/properties/components/Popover.json * Update tokens/properties/components/Popover.json * Update tokens/properties/components/Popover.json * Update tokens/properties/components/Popover.json * Update tokens/properties/components/Popover.json * Update tokens/properties/components/Popover.json * feat: add Popover component (#373) * feat: add Popover component The scss use directive has to be added to vue.config.js because it has to be in front of all other rules. Co-authored-by: bereket-WMDE <71328938+bereket-WMDE@users.noreply.github.com> * Fix most rtl positioning issues Easiest would be to manually define two explicit rtl and ltr blocks to workaround transforms not being handled by postcss-logical, but that doesn't work because we're prefixing _everything_ with .wikit :/ So we need to figure out how to not rely on translateX and horizontally moved origins of transformation. * Fix alignment of horizontal center tooltips translateX isn't adjusted by postcss-logical, therefore we have to make use of flexbox also somewhat affecting its absolute positioned children. * Fix story layout remove debugging color and make headline be on its own line. * Change left to start and right to end for Popover This accounts for the Popover actually switching sides between ltr and rtl. * Set direction in story explicitly to fix Safari Co-authored-by: bereket-WMDE <71328938+bereket-WMDE@users.noreply.github.com> Co-authored-by: Michael Große <michael.grosse@wikimedia.de> Co-authored-by: bereket-WMDE <71328938+bereket-WMDE@users.noreply.github.com>

micgro42 force-pushed the PopoverComponent branch from 35f87d6 to 7334b46 Compare February 11, 2021 17:28

bereket-WMDE reviewed Feb 15, 2021

View reviewed changes

micgro42 force-pushed the PopoverComponent branch 7 times, most recently from 5e33729 to d7ef54c Compare February 17, 2021 16:02

bereket-WMDE approved these changes Feb 17, 2021

View reviewed changes

micgro42 force-pushed the PopoverComponent branch 4 times, most recently from 3b8efb3 to 761e22b Compare March 1, 2021 11:32

micgro42 force-pushed the PopoverComponent branch 2 times, most recently from ed56a2f to 29600bc Compare March 1, 2021 14:57

feat: add Popover component

cd15568

The scss use directive has to be added to vue.config.js because it has to be in front of all other rules. Co-authored-by: bereket-WMDE <71328938+bereket-WMDE@users.noreply.github.com>

micgro42 force-pushed the PopoverComponent branch from 29600bc to cd15568 Compare March 1, 2021 16:39

Fix alignment of horizontal center tooltips

ea42037

translateX isn't adjusted by postcss-logical, therefore we have to make use of flexbox also somewhat affecting its absolute positioned children.

micgro42 added 2 commits March 2, 2021 10:46

Fix story layout

83d9952

remove debugging color and make headline be on its own line.

Change left to start and right to end for Popover

3ee9675

This accounts for the Popover actually switching sides between ltr and rtl.

Set direction in story explicitly to fix Safari

5ee4ec4

sai-san merged commit 43bd206 into create-popover-tokens Mar 2, 2021

sai-san deleted the PopoverComponent branch March 2, 2021 16:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add Popover component #373

feat: add Popover component #373

micgro42 commented Feb 11, 2021

github-actions bot commented Feb 11, 2021

Ladsgroup commented Feb 12, 2021

bereket-WMDE left a comment •

edited

micgro42 commented Feb 15, 2021

bereket-WMDE commented Feb 15, 2021

sai-san commented Feb 26, 2021

sai-san commented Feb 26, 2021 •

edited

micgro42 commented Feb 26, 2021

micgro42 commented Feb 26, 2021 •

edited

sai-san commented Mar 1, 2021 •

edited

micgro42 commented Mar 1, 2021

micgro42 commented Mar 1, 2021

sai-san commented Mar 1, 2021

micgro42 commented Mar 1, 2021

micgro42 commented Mar 1, 2021

micgro42 commented Mar 1, 2021

micgro42 commented Mar 2, 2021

sai-san commented Mar 2, 2021 •

edited

micgro42 commented Mar 2, 2021

micgro42 commented Mar 2, 2021

sai-san commented Mar 2, 2021

sai-san commented Mar 2, 2021

micgro42 commented Mar 2, 2021

sai-san commented Mar 2, 2021

micgro42 commented Mar 2, 2021

micgro42 commented Mar 2, 2021

feat: add Popover component #373

feat: add Popover component #373

Conversation

micgro42 commented Feb 11, 2021

github-actions bot commented Feb 11, 2021

Ladsgroup commented Feb 12, 2021

bereket-WMDE left a comment • edited

Choose a reason for hiding this comment

micgro42 commented Feb 15, 2021

bereket-WMDE commented Feb 15, 2021

sai-san commented Feb 26, 2021

sai-san commented Feb 26, 2021 • edited

micgro42 commented Feb 26, 2021

micgro42 commented Feb 26, 2021 • edited

sai-san commented Mar 1, 2021 • edited

micgro42 commented Mar 1, 2021

micgro42 commented Mar 1, 2021

sai-san commented Mar 1, 2021

micgro42 commented Mar 1, 2021

micgro42 commented Mar 1, 2021

micgro42 commented Mar 1, 2021

micgro42 commented Mar 2, 2021

sai-san commented Mar 2, 2021 • edited

micgro42 commented Mar 2, 2021

micgro42 commented Mar 2, 2021

sai-san commented Mar 2, 2021

sai-san commented Mar 2, 2021

micgro42 commented Mar 2, 2021

sai-san commented Mar 2, 2021

micgro42 commented Mar 2, 2021

micgro42 commented Mar 2, 2021

bereket-WMDE left a comment •

edited

sai-san commented Feb 26, 2021 •

edited

micgro42 commented Feb 26, 2021 •

edited

sai-san commented Mar 1, 2021 •

edited

sai-san commented Mar 2, 2021 •

edited