These are chat archives for reactioncommerce/reaction

17th
Apr 2015
Aaron Judd
@aaronjudd
Apr 17 2015 01:42
@spencern I don’t think there is an issue for it -> probably just the discussion in #144. I think it’s needed, and look forward to seeing what you come up with (an issue would be great)
Aaron Judd
@aaronjudd
Apr 17 2015 01:50
@spencern the other thing I have been thinking about is moving the build dependency out of reaction-core so that reaction-core-theme could be installed independently and another theme could be used - I'm not sure if you have ideas or improvements to that
Spencer Norman
@spencern
Apr 17 2015 02:08
I think it's worth discussing. Another thing that's worth a talk at some point would be considering moving the templates into their own package. Not sure how straight forward it is it override them in other packages, but obviously I'm changing all of them because zurb classes are significantly different from bootstrap
Aaron Judd
@aaronjudd
Apr 17 2015 05:22
:point_up: April 16 2015 4:29 PM added #370 -> we can add to that as needed
@spencern easy to replace with Template.xxx.replaces(“xxx”) - the main reason I’ve kept them in core is the idea that one package needs to be fully functional, without any additional requirements other than a theme. - that and managing packages has been a challenge (early on I had it even more modular)- although, the package management stuff is getting better - I think it might be worth reconsidering soon
Goutham Veeramachaneni
@gouthamve
Apr 17 2015 16:03
Hmm, Even Phantomjs conversion is bad. I used this npm package:https://www.npmjs.com/package/html-pdf and tried it out (normal node no meteor) to convert the html blaze gives. It was only slightly better than jsPDF.
I think the best solution is the browser print function
Aaron Judd
@aaronjudd
Apr 17 2015 16:44
yeah, I’m afraid that’s the conclusion I reached as well
@spencern I’ve been following how @raix and @aldeed refactored the CFS repo, where packages are included in the main repo.. I’ve been keeping an eye on that, see how those guys feel about the refactor
Spencer Norman
@spencern
Apr 17 2015 16:55
Yeah, that's a great example to follow I think (CFS). I think as I get further along with adding Foundation I'll have a better perspective on where it will be nice to extend core and where it might need to change. Good to know that the templates are simple to override.
Aaron Judd
@aaronjudd
Apr 17 2015 16:58
they’re easy to replace, but it’s not like they are object oriented, so I can see some potential hierachy issues - I’m not sure what happens if you apply ‘replaces’ multiple times to the same template - potentially issues - generally I think it’s probably wise to only use replaces for the app and not the packages..
but that might be something you can get a better feel for as you are building your Foundation package
Spencer Norman
@spencern
Apr 17 2015 17:00
so I shouldn't plan on using replaces for the scss/foundation package?
Aaron Judd
@aaronjudd
Apr 17 2015 17:01
I’m assuming last one wins.. (which I think would always be the app level) .. but what I am pointing out is if you do ‘replaces’ and another package also uses ‘replace’ what’s the outcome?
Spencer Norman
@spencern
Apr 17 2015 17:01
I'll do some experimenting, but that's good to consider.
Aaron Judd
@aaronjudd
Apr 17 2015 17:03
I’m just suggesting you try it, but think about that scenario. I’m tempted to say - “well you then have your unique template names for foundation, and if another app or packages wishes to use those - that’s intentional” - so I suggest you use it and just think of the re-use scenarios
I’ve also been wanting to rename all the core methods and templates for a while now - to give some naming formal conventions, ie: (“coreLayout” vs “Layout”). There is some conventions already - just not formal and enforced.. you’ll probably have some ideas there as well
maybe as simple as prefix all templates with the package name
(or we could get sneaky and introduce some of our own auto namespacing)
Aaron Judd
@aaronjudd
Apr 17 2015 17:09
re: method renaming: “core/copyOrdertoCart” was the direction I was thinking
Spencer Norman
@spencern
Apr 17 2015 17:13
Namespacing templates will help a lot with this. Do all templates get sent to the client regardless of whether or not they get used?
Aaron Judd
@aaronjudd
Apr 17 2015 17:36
yes (but loading only what is needed is something that’s being discussed in the community)
Bogi
@boboci9
Apr 17 2015 17:49

Hi @aaronjudd, I think I found the source of the issues #350, is could be coming from the unless line:

  unless ReactionCore.Collections.Packages.find().count() is Object.keys(ReactionRegistry.Packages).length
    _.each ReactionRegistry.Packages, (config, pkgName) ->
      Shops.find().forEach (shop) ->
        ReactionCore.Events.info "Initializing "+ pkgName
        ReactionCore.Collections.Packages.upsert {shopId: shop._id, name: pkgName},
          $setOnInsert:
            enabled: !!config.autoEnable
            settings: config.settings
            registry: config.registry
            shopPermissions: config.permissions
            services: config.services

form the fixutres.coffee

Bogi
@boboci9
Apr 17 2015 17:58
This message was deleted
when there are more shops this Packages.find().count() is always multiplied by the shop numbers
Aaron Judd
@aaronjudd
Apr 17 2015 18:11
doesn’t that work towards having packages per shop?
happy to take a PR if you resolve
Bogi
@boboci9
Apr 17 2015 18:15
ok, I will have to add some other small changes too like adding shopId in the reactionApps filter
Template.registerHelper "reactionApps", (options) ->
  reactionApps = []
  filter = {}
  registryFilter = {}
  # any registry property, name, enabled can be used as filter
  for key, value of options.hash
    unless key is 'enabled' or key is 'name' or key is 'shopId'
...
Aaron Judd
@aaronjudd
Apr 17 2015 18:15
ok, that makes sense
Bogi
@boboci9
Apr 17 2015 18:15
I will have to test it a little more but I will put together a PR at the beginning of next week
Aaron Judd
@aaronjudd
Apr 17 2015 18:16
:thumbsup:
are you using Shops.json for fixture data? would be nice to add a second shop, to the data, but disabled, for testing
I can do that, after your PR -> just curious
Bogi
@boboci9
Apr 17 2015 18:18
I have several other shops in my DB because I am testing what changes will be needed for the multi vendor
Goutham Veeramachaneni
@gouthamve
Apr 17 2015 18:23
@aaronjudd should I send a PR implementing the browser print and remove the jsPDF dependency and package? I have a useable template for invoices working with browser print now.
Aaron Judd
@aaronjudd
Apr 17 2015 18:24
@Gouthamve yes, that would be great. I think that’s the way to go (at least for now)
Goutham Veeramachaneni
@gouthamve
Apr 17 2015 18:50
@aaronjudd Sent it, while the html might not be good (I suck at front end) The printed page could be used right away. Also looking for feedback on the new routeController for Printing pages.
Aaron Judd
@aaronjudd
Apr 17 2015 18:52
ok, get ready to head out for lunch, I’ll take a look when I get back. understood on the styling. I’m looking to get some awesome theme developers to give everything a nice polish…soon..
Goutham Veeramachaneni
@gouthamve
Apr 17 2015 19:20
Okay, I take it back. I just tested it on safari and assumed it would work everywhere sorry. On chrome and firefox, its bad.
Even browser print might be problem. Or maybe it was my html.
safari.pdf
What you see above is done using safari. The same in chrome/firefox:
chrome.pdf
I am going to play around with the html a little bit to see if I can get it working in chrome.
Spencer Norman
@spencern
Apr 17 2015 19:25
Are you using divs/position/floats or tables to lay this out?
Goutham Veeramachaneni
@gouthamve
Apr 17 2015 19:27
divs and bootstrap columns.
I know only the basic bootstrap but I am looking to learn. So any suggestions are welcome.
Spencer Norman
@spencern
Apr 17 2015 19:54
For print, I'd suggest actually using a static table design rather than anything responsive. It looks to me like Chrome/firefox are presenting a smaller viewport or something for the printer and it's causing the responsive bootstrap columns to stack.
The other option you could try would be making sure you are specifying that the columns are for small or xs devices. Not really sure what chrome and firefox have for their print settings, but that definitely appears to be the problem.
Goutham Veeramachaneni
@gouthamve
Apr 17 2015 20:02
Hmm, adding xs definitely works. Thanks. Will update the template.
Goutham Veeramachaneni
@gouthamve
Apr 17 2015 20:44
Lol,=> Client modified -- refreshing (x55) I had to do small changes then wait for it to reload, look at the print preview and do it all over again.
Thanks @spencern. Works everywhere now.