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

update ControlPosition #71

Merged
merged 4 commits into from
Nov 9, 2023

Conversation

MauricioRobayo
Copy link
Contributor

@MauricioRobayo MauricioRobayo commented Nov 9, 2023

Updates the ControlPosition enum according to this: #67 (comment)

Fixes #67

@usefulthink
Copy link
Collaborator

Awesome, thanks a lot. Merging this right away.

@usefulthink
Copy link
Collaborator

Dang, I just noticed that the values are duplicated for aliases (TOP === TOP_CENTER), so this doesn't work as an enum. We could either remove the duplicates and keep the enum or declare this as an const object. What do you think?

@MauricioRobayo
Copy link
Contributor Author

I don't love enums, so I'm fine with making it an object if that works for you. Will come back to this later today.

@usefulthink
Copy link
Collaborator

Perfect, let's do this.

BLOCK_END_INLINE_CENTER: 24,
BLOCK_END_INLINE_END: 25
} as const;
export type ControlPosition =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about the naming convention here, should I use a different name to distinguish between the type and the object?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've seen that recommended elsewhere and it seems to work exactly like the enums, so let's do the same name..

@usefulthink usefulthink merged commit 1dd144a into visgl:main Nov 9, 2023
2 checks passed
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.

ControlPosition not positioning where it should
2 participants