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/v4 compatibility #16

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fix/v4 compatibility #16

wants to merge 6 commits into from

Conversation

gltarsa
Copy link

@gltarsa gltarsa commented Jan 14, 2017

The WF2 nav bar needs to use the link_html feature of the simple-navigation gem, but that feature was added in V4 and our Gemfile is locked at V3 because it uses the simple-navigation-bootstrap gem which has not had a maintainer since 2015 and does not support V4. There is a PR against the *-bootstrap source that updated the gem for the V4 changes, but it introduced a bug in the icon: code.

This PR incorporates that V4 update PR and fixes the icon: bug. The intent is to maintain this gem privately until such time as a maintainer comes forward.

@pdf
Copy link
Owner

pdf commented Jan 14, 2017

Can you confirm whether split navigation and dropdowns still function correctly with v4.x? Please drop the version bump commit - I'll need to bump the major version since we'll be changing the simple-navigation dependency, and there is a rake task that will handle this for release.

@gltarsa
Copy link
Author

gltarsa commented Jan 14, 2017

If by "split navigation" you mean "sub-navigation," then yes, both features still function correctly with v4.x because I am using them both in our code. I have reverted the version file to the current master version, so only two files are now changed.

@pdf
Copy link
Owner

pdf commented Jan 14, 2017

If you take a look at the README, there's a split navigation feature that creates a separate dropdown toggle, so that the primary button can function with a link. I just have no way to test this easily right now.

@gltarsa
Copy link
Author

gltarsa commented Jan 14, 2017

Working on this now. I have run into an unrelated problem with my app during the test. I'll let you know shortly when the test is complete.

@gltarsa
Copy link
Author

gltarsa commented Jan 14, 2017

The test will not be happening tonight.

@pdf
Copy link
Owner

pdf commented Jan 14, 2017

No problem, take your time.

Prior to this commit split:true was not working properly, nor were
dividers.  This commit fixes a number of places where the structure of
the Item class changed between versions, including changes where
attributes which were not exposed were accessed.
Thor versions 19.2-19.4 generate these warning messages:

  Expected string default value for '--helper'; got true (boolean)
  Expected string default value for '--assets'; got true (boolean)
  Expected string default value for '--decorator'; got true (boolean)
  Expected string default value for '--decorator'; got true (boolean)
  Expected string default value for '--serializer'; got true (boolean)

Thor is used in rails and jquery-ui, among other gems.  None of them
locks the version.

This commit requests at least version 19.1 and refuses to load .2, .3 or
.4.  The assumption is that this will be fixed in the next release and
this fix should only prove a problem if it is not AND some other gem is
updated.
@@ -20,4 +20,6 @@ Gem::Specification.new do |s|
s.add_development_dependency "rake"
s.add_runtime_dependency "simple-navigation", ">= 4.0.0"
s.add_runtime_dependency "railties", ">= 3.1"
# thor dependencies assume the spurious warning messages bug will be fixed 19.5
s.add_runtime_dependency "thor", "0.19.1", "!=0.19.2", "!=0.19.3", "!=0.19.4"
Copy link
Owner

Choose a reason for hiding this comment

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

Should probably only be a development dependency.

@pdf
Copy link
Owner

pdf commented Jan 21, 2017

Do you suspect this is close to ready for review?

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