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

Use vulcan-ui-bootstrap (issue #50) #51

Merged
merged 5 commits into from
May 4, 2018
Merged

Conversation

kabalin
Copy link
Contributor

@kabalin kabalin commented Apr 27, 2018

Here are the patches for example-forms and example-membership. @SachaG I am not sure if you done example-forum already, if not forum patches will follow here.

@kabalin
Copy link
Contributor Author

kabalin commented Apr 27, 2018

One issue I came across though, is that adding vulcan:ui-bootstrap to package.js in example package results in a error that ui-bootstrap package is not found. Using meteor add vulcan:ui-bootstrap results in a same error. This is clearly not the case for other vulcan:* packages used in package.js in other examples, e.g. example-forum imports many different vulcan packages. I am using two-repos mode with METEOR_PACKAGE_DIRS env pointing to ../Vulcan/packages location.

I only managed to resolve the issue by specifying vulcan:ui-bootstrap explicitly in .meteor/packages, i.e.

diff --git a/.meteor/packages b/.meteor/packages
index 6590856..b9151ab 100644
--- a/.meteor/packages
+++ b/.meteor/packages
@@ -1,6 +1,7 @@
 # see http://docs.vulcanjs.org/packages
 
 vulcan:core@1.9.0
+vulcan:ui-bootstrap@1.9.1

Not sure if this is something with my setup, or am I missing something?

@kabalin
Copy link
Contributor Author

kabalin commented Apr 27, 2018

Added patch for example-forum without Dropdown and Modal refactoring yet.

@SachaG
Copy link
Contributor

SachaG commented Apr 28, 2018

I only managed to resolve the issue by specifying vulcan:ui-bootstrap explicitly in .meteor/packages, i.e.

Yes, I've been running into the same issue. I think it's a problem with Meteor's package management system.

In this specific case though we don't want to add the dependency in the example package's package.js files though, since that would mean users wouldn't be able to switch from bootstrap to other UI frameworks without modifying the file. So putting it in .meteor/packages is the right move I think.

I'll wait until you're finished with the dropdown/modal stuff to merge. Don't hesitate to ping me on Slack if the dropdown stuff is confusing btw :)

@kabalin
Copy link
Contributor Author

kabalin commented Apr 28, 2018

In this specific case though we don't want to add the dependency in the example package's package.js files though, since that would mean users wouldn't be able to switch from bootstrap to other UI frameworks without modifying the file. So putting it in .meteor/packages is the right move I think.

In fact, I would keep reference to ui-bootstrap in example package's package.js. You know, these packages are "examples" by defintion, someone willing to use other UI framework probably would end up modifying the code anyway. Though, I can remove package.js changes from the patches if you wish.

I looked closer at modal and have got a question. UsersProfileCheck.jsx has Modal.Footer, while Components.ModalTrigger does not have one. Shall I make ModalTrigger to support footer or move its content to body when migrating to Components.ModalTrigger (or even remove footer content completely)?

@SachaG
Copy link
Contributor

SachaG commented Apr 29, 2018

You know, these packages are "examples" by defintion, someone willing to use other UI framework probably would end up modifying the code anyway.

Yes, that's a good point as well. Let's leave it in package.js for now then.

Shall I make ModalTrigger to support footer or move its content to body when migrating to Components.ModalTrigger (or even remove footer content completely)?

Let's remove the footer for now then. We can start simple and always add more features if they're needed in more than one place.

kabalin and others added 5 commits May 1, 2018 01:46
Moving away of direct react-bootstrap dependency (issue VulcanJS#50).
Moving away of direct react-bootstrap dependency (issue VulcanJS#50).
Moving away of direct react-bootstrap dependency (issue VulcanJS#50).

Patch dos not include Dropdown and Modal refactoring yet.
Moving away of direct react-bootstrap dependency (issue VulcanJS#50).
@kabalin
Copy link
Contributor Author

kabalin commented May 1, 2018

The Modal patch 4b58510 depends on VulcanJS/Vulcan#1971 In fact, I kept footer since I ended up adding a separate component for modal.

Dropdown changes will follow later this week.

@SachaG
Copy link
Contributor

SachaG commented May 2, 2018

Can you push what you have so far? I can take care of the rest :)

@kabalin
Copy link
Contributor Author

kabalin commented May 2, 2018

@SachaG I did not touch Dropdown code yet to be honest (steep learning curve you know), so there are no pending changes. All I have done so far is pushed, so feel free to take it over from here if you wish :)

@SachaG
Copy link
Contributor

SachaG commented May 3, 2018

OK, I'll try to do the dropdown stuff today then. Thanks for your work so far!

@SachaG SachaG merged commit 4b58510 into VulcanJS:master May 4, 2018
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