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

simplify the setting of button text: { okText: "foo", cancelText: "bar" ... #52

Closed
wants to merge 1 commit into from

Conversation

XadillaX
Copy link

simplify the setting of button text: { okText: "foo", cancelText: "bar" }, and fix a bug that may occur an error like: 'Invalid GBK character "\xE2"' when compiling vex.sass.

@adamschwartz
Copy link
Contributor

Hey @XadillaX thanks! This definitely seems like a worthy addition.

A few questions / concerns:

  1. Do you find that you're needing to change the text very often per individual dialog?

    If not, you can already set these globally with the following JS:

    vex.dialog.buttons.YES.text = 'foo'
    vex.dialog.buttons.NO.text = 'bar'
  2. If yes, and we decide to add these properties, since these buttons are already called YES and NO, I'd like to either make them yesText and noText, or change the buttons to OK and CANCEL. Which would be preferable? Need to think about this.

  3. For the Invalid GBK character error, judging from these two GitHub issues, this may not be a real error. I myself cannot reproduce locally.

  4. As for the actual implementation, if options.okText and options.buttons[0] and typeof options.okText is 'string' doesn't seem like quite the right check to me. I think we actually want to check that buttons wasn't passed in too (by saving the original passedOptions before they're extended into the defaults) and possibly additionally that vex.dialog.buttons was never changed from the factory default globally.

@XadillaX
Copy link
Author

Hi Adam:

I know I can set text via { buttons.YES.text }, I just think it will be a little too long. I think { okText(or yesText) } is more simple. If I use vex as a "library", I prefer to use API than modifying source code, so I want an API for simplify text settings.

And I think both yesText or okText will be OK. I used okText just because the default text is OK and Cancel.

For third problem, my environment is Windows 7 (Simplified Chinese), ruby1.9.3. And I found the answer here and here. That means some environment's default encoding is GBK or something others, so we should set the encoding at the top of sass file.

For the last question, I just followed least modification. Or I'll change the default structure of defaultOptions.

After we decide the questions, I think you or me can do some modifies to meet the best solution. But I think I have to go to bed now. Maybe I can read email or review the code at bed via my phone.

@bbatliner
Copy link
Contributor

bbatliner commented Feb 24, 2018

Check out #10, which has this functionality exactly. Vex will update its dep on vex-dialog soon to include these new features. (see #258)

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.

3 participants