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 strange popover positioning #3789

Merged
merged 1 commit into from Sep 22, 2023

Conversation

manuelmeister
Copy link
Member

The wording of the popover positioning was very irritating.

You now specify the desired position of the activator.
I also added a default activator and warning text. This will solve 80% of the use cases.

Bildschirmfoto 2023-09-16 um 16 08 04

Comment on lines 86 to 95
positions.left = true
} else if (this.x === 'right') {
if (this.activatorX === 'left') {
positions.right = true
} else if (this.activatorX === 'right') {
positions.left = true
}
if (this.y === 'top') {
positions.top = true
positions.nudgeBottom = 10
} else if (this.y === 'bottom') {
if (this.activatorY === 'above') {
positions.bottom = true
} else if (this.activatorY === 'below') {
positions.nudgeBottom = 10
positions.top = true
}
Copy link
Member

Choose a reason for hiding this comment

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

I think I found it more intuitive before. You now added activator-x and activator-y props. But before, these were not the position of the activator relative to the popover, but the position of the popover relative to the activator. When I use the PopoverPrompt component, I primarily think about where in the UI I put the activator, and then from there, on which side the prompt should pop up once the user clicks the activator. I think that was the way it was implemented before (but I didn't test it out).

If that confuses you, how about renaming the existing x and y props to popover-x and popover-y, or removing our custom position props entirely and just expose Vuetify's left, right, top and bottom props?

Copy link
Member Author

Choose a reason for hiding this comment

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

I always thought about the origin point and never about a target point. What about origin-x and origin-y?

I would not want to remove our custom position props as someone could define left right and top at the same time. I need to know in which edge the origin with the activator button is. I don't want to provide more options.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand what "origin" means in this context. In my mental model, an origin can't be shifted, it is usually at (0, 0), and therefore origin-x and origin-y don't make sense to me.
Terms I would understand in this context include "direction", "menu-position", "popover", "activator" (although I don't think new devs will easily understand that), "opener", "button", "trigger", "dropdown", ...

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking about the transform origin (css: transform-origin: top right;), which is the point where the popover scales up from. Aligning the popover and button to the same axis makes it more intuitive.

If I understand correctly, for the picture in the issue description, you would define x as left and y as below.
Because, to me, the button and popover are aligned to the right.

@usu what do you think? If you agree with Carlo, I will change the attributes to be defined like this:

button="right"
popover="below"

(for the picture in the issue description)

Copy link
Member Author

Choose a reason for hiding this comment

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

@carlobeltrame I looked at this again and agree that it was not well worded. I also noticed that Vuetify v-menu has some strange quirk with its left and right prop:
https://codepen.io/manuelmeister/pen/wvRpqLj

When I first implemented this, this confused me, because in CSS if something is aligned right then it is aligned to the right side of container and not "from the right side overflowing towards left" 🤷

I have settled for

button-align="left"
popover-position="top"

which will result in

╭─────────────────────────────────╮ 
│                                 │
│             popover             │
│                                 │
├──────────┬──────────────────────╯
│  button  │
╰──────────╯

@manuelmeister manuelmeister added the deploy! Creates a feature branch deployment for this PR label Sep 21, 2023
@manuelmeister manuelmeister temporarily deployed to feature-branch September 21, 2023 08:44 — with GitHub Actions Inactive
@github-actions
Copy link

github-actions bot commented Sep 21, 2023

Feature branch deployment currently inactive.

If the PR is still open, you can add the deploy! label to this PR to trigger a feature branch deployment.

@manuelmeister manuelmeister temporarily deployed to feature-branch September 21, 2023 11:12 — with GitHub Actions Inactive
Copy link
Member

@carlobeltrame carlobeltrame left a comment

Choose a reason for hiding this comment

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

It's definitely better now than the way you had it before.
I still think would be more intuitive to have both position props refer to the same element: popover-align and popover-position, since the popover position on the screen is relative to the activator, not the other way round. But if you don't agree, the wording of the props is now clearer than x and y, so I'm fine with the current state.

frontend/src/locales/fr.json Outdated Show resolved Hide resolved
@manuelmeister
Copy link
Member Author

@carlobeltrame I changed it to align and position.

@manuelmeister manuelmeister temporarily deployed to feature-branch September 21, 2023 18:14 — with GitHub Actions Inactive
@manuelmeister manuelmeister added this pull request to the merge queue Sep 22, 2023
Merged via the queue into ecamp:devel with commit 2d16f29 Sep 22, 2023
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy! Creates a feature branch deployment for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants