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

Option for specifying a dynamic width #329

Merged
merged 1 commit into from
Sep 11, 2020

Conversation

dopatraman
Copy link
Contributor

Goal:
The goal of this PR is to provide users with an option to specify that their UI features a dynamic width.

Reason for change:
The application I am building expands the width of the document element dynamically. As a result, the anchor point of the dropdown stays at the window width's edge instead of expanding rightward.

Since the clientWidth of the document is computed initially and never again, it isn't ideal for applications whose UI expands. Instead, scrollWidth works better. This PR adds an option for specifying a need for this value.

Copy link
Owner

@yuku yuku left a comment

Choose a reason for hiding this comment

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

@dopatraman Thanks for sending the PR! Please check my comment.

@@ -204,7 +205,7 @@ export class Dropdown extends EventEmitter {
if (doc) {
const elementWidth = this.el.offsetWidth
if (cursorOffset.left) {
const browserWidth = doc.clientWidth
const browserWidth = this.options.dynamicWidth ? doc.scrollWidth : doc.clientWidth
Copy link
Owner

Choose a reason for hiding this comment

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

I'm wondering if there are other potentially available value here. So I'd like to support a function parameter something like ↓ instead of a boolean parameter for flexibility:

new Textcomplete(editor, [strategy], {
  dropdown: {
    getParentWidth: (el) => el.scrollWidth // default:  (el) => el.clientWidth
  }
})

Copy link
Contributor Author

@dopatraman dopatraman Sep 10, 2020

Choose a reason for hiding this comment

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

There's an issue with this approach: the element passed in will always be the document element, and it will be hard coded into the library. So what we end up with is not much flexibility and a more confusing interface:

const browserWidth = this.options.getParentWidth ? this.options.getParentWidth(doc) : doc.clientWidth;                                                         

Thoughts?

Copy link
Owner

Choose a reason for hiding this comment

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

ic, finally this patch seems good for me. I'm going to merge.

Copy link
Owner

@yuku yuku Sep 11, 2020

Choose a reason for hiding this comment

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

Oops, there isn't this.options. (There is this.option 🤯)

@yuku yuku merged commit d338eb3 into yuku:main Sep 11, 2020
@dopatraman dopatraman deleted the dynamic-width-option branch September 11, 2020 16:33
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