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

Improvements #20

Merged
merged 24 commits into from
Nov 11, 2021
Merged

Improvements #20

merged 24 commits into from
Nov 11, 2021

Conversation

indivisualvj
Copy link
Contributor

i use it for my VJ project but i found some issues.

@indivisualvj indivisualvj changed the title Aprovements Improvements Nov 9, 2021
@colejd
Copy link
Owner

colejd commented Nov 9, 2021

Thanks for the PR! Could you please give me an overview of what you've added in terms of features? The bugfixes are pretty straightforward, but I'm having trouble understanding what your search commit did, for example.

@colejd
Copy link
Owner

colejd commented Nov 9, 2021

Also, did you make any breaking changes here?

@indivisualvj
Copy link
Contributor Author

Thanks for your attention!
guify is the best alternative i found to manage applications with LOTS of settings.
In my view i enhanced search behaviour to be more smooth/less greedy or nervous.
Also if you press enter from within the search field, the first option left will be triggered.
This can be a:

  • checkbox (being checked/unchecked)
  • select (catch focus and expand)
  • text (catch focus)
  • range (catch focus)
  • ...

@indivisualvj
Copy link
Contributor Author

indivisualvj commented Nov 9, 2021

I made major changes on the MenuBar. The "Controls" Button is now a Search Input. You can search for settings here. And then it will activate on Enter.
Also The MenuBar is now clickable so that you do not need the "Controls" Button anymore.

@colejd
Copy link
Owner

colejd commented Nov 9, 2021

Gotcha, thank you. I'd respectfully request that you remove your menu bar search code from this PR. My reasoning is that I actually like the search bar you added, funny enough! I see a use case for being able to add arbitrary components to the menu bar, and in your case, it would be a text component.

So there's opportunity for another PR that does two things:

  • Allows arbitrary components to be added to the menu bar (would need to figure out how to make that look good).
  • Allows any text field to set a timeout, which makes the text field more useful for search (as you've done in this PR).

That way, to get search like you have in this PR, you can just add a text component to the menu bar and hook up the handlers in the same way you'd hook up any other component. Does that sound reasonable for your use case?

src/components/checkbox.js Outdated Show resolved Hide resolved
src/menu-bar.js Outdated Show resolved Hide resolved
@colejd
Copy link
Owner

colejd commented Nov 9, 2021

I've updated the library to 0.13.1. I made several changes to range.js, which were built on top of your branch. So to fix your PR, you'll just need to rebase keeping my copy of range.js.

@indivisualvj
Copy link
Contributor Author

Thank you. That all seems to make sense. I'm always happy if someone takes care about doing things well thought out. Will have a look at all of that soon.

Copy link
Owner

@colejd colejd left a comment

Choose a reason for hiding this comment

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

Looking good, just a few more small things to address.

.gitignore Outdated Show resolved Hide resolved
src/components/button.js Outdated Show resolved Hide resolved
src/components/select.js Outdated Show resolved Hide resolved
src/components/text.js Outdated Show resolved Hide resolved
src/gui.js Outdated Show resolved Hide resolved
@colejd colejd added this to In progress in v0.14.0 Nov 10, 2021
@colejd colejd moved this from In progress to Review in progress in v0.14.0 Nov 10, 2021
@colejd
Copy link
Owner

colejd commented Nov 10, 2021

Apologies for the conflicts. I made some more changes on my side. Resolving is fairly easy again though -- you can fix by doing the following:

  1. Merge my version of master into your branch
  2. Resolve conflicts for any of the files in src/components by accepting my changes
  3. Run npm run buildall, then resolve merge conflicts in the lib folder by accepting your changes

(These changes update dependencies and build commands, by the way. Run npm install to get the new ones, and check out the readme for details)

# Conflicts:
#	lib/guify.js
#	lib/guify.js.map
#	lib/guify.min.js
#	src/components/button.js
#	src/components/checkbox.js
#	src/components/color.js
#	src/components/display.js
#	src/components/interval.js
#	src/components/select.js
#	src/components/text.js
reverted select element value behaviour
reverted onChange only fired if value changed
Copy link
Owner

@colejd colejd left a comment

Choose a reason for hiding this comment

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

I think your merge overwrote my new changes to master, so I'm going to revert them here.

src/components/button.js Outdated Show resolved Hide resolved
src/components/checkbox.js Outdated Show resolved Hide resolved
src/components/color.js Outdated Show resolved Hide resolved
src/components/color.js Outdated Show resolved Hide resolved
src/components/color.js Outdated Show resolved Hide resolved
src/components/display.js Outdated Show resolved Hide resolved
src/components/interval.js Outdated Show resolved Hide resolved
src/components/interval.js Outdated Show resolved Hide resolved
src/components/select.js Outdated Show resolved Hide resolved
src/components/text.js Outdated Show resolved Hide resolved
Copy link
Owner

@colejd colejd left a comment

Choose a reason for hiding this comment

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

Looking good. Thanks for all your help here!

v0.14.0 automation moved this from Review in progress to Reviewer approved Nov 11, 2021
@colejd colejd merged commit 4fdd42e into colejd:master Nov 11, 2021
v0.14.0 automation moved this from Reviewer approved to Done Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants