Skip to content
This repository has been archived by the owner on Apr 30, 2018. It is now read-only.

Input add-ons #5

Closed
wants to merge 9 commits into from
Closed

Input add-ons #5

wants to merge 9 commits into from

Conversation

Ledragon
Copy link
Contributor

New wrapper for input add-ons on left and right of fields, through text or css class

@kentcdodds
Copy link
Member

Awesome! Just a few changes that I'll note when I get to a computer. Thanks!

@Ledragon
Copy link
Contributor Author

Glad I could help, I feel so useful now :D

@@ -0,0 +1,5 @@
## 2015-02-19
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding a Changelog. Good to have. Could you change this line to:

# 3.0.2

@kentcdodds
Copy link
Member

You know what. I think I would prefer to make this a template manipulator rather than a wrapper. So you'd do something like this:

Place the file in a new directory called other or something (doesn't make a difference to me), then add this code:

angular.module('formlyBootstrap').run(function(formlyConfig, $http, $templateCache) {
  formlyConfig.templateManipulators.postWrapper.push(function(template, options) {
    if (!options.templateOptions.addonLeft && !options.templateOptions.addonRight) {
      return template;
    }
    return $http.get('other/formly-templates-bootstrap-addons.html', {
      cache: $templateCache
    }).then(function(response) {
      return response.data.replace('<formly-transclude></formly-transclude>', template);
    });
  });
});

Does that make sense?

@kentcdodds
Copy link
Member

The reason I would do this is that way this only applies to fields that have an addonLeft and/or an addonRight and doesn't apply to any other fields which will save on watchers and dom nodes. Let me know what you think.

@kentcdodds
Copy link
Member

I'm adding this example to the formly website. Hopefully it's helpful :-)

@Ledragon
Copy link
Contributor Author

I will try to grasp it in the upcoming days and understand better what the difference between the two approaches are. In all honesty, I am not a great fan of the way I manage all thos ng-if that make the code quite hard to read. The approach you suggest would make the html easier to read, and simplify things, so that seem good to me.
It would certainly be more appropriate to use it only for input, maybe textarea. Select might be of interest too, thought it looks funny if you put an add-on on the right.
As for the empty span, the intent is to provide classes such as glyphicon from bootstrap or fa from font-awesome. I put it like this as this is what I do most of the time.
I just changed the version in the changelog.md.
Thanks for the feedback.

@kentcdodds
Copy link
Member

Ah, understood. I usually use <i> tags for icons, but it really doesn't matter. Thanks for your help. Looking forward to updates! Thanks for your contributions!

@Ledragon
Copy link
Contributor Author

I could change it to an <i>, no problem with that. I will try my best to keep on it soon. My pleasure to finally contribute to an active project :-).

@Ledragon
Copy link
Contributor Author

How can I restrict the template replacement to input only? Following your suggestion, I think it makes more sense to allow add-ons for input only, and maybe extend it for other controls at a later stage.

span replaced by i for icon
use of template replacer
@kentcdodds
Copy link
Member

At the top of your function, check that options.type === 'input'. That should do it :-)

@Ledragon
Copy link
Contributor Author

I made the updates according to the discussion: replaced span by i and use of templateManipulator.
I used preWrapper instead of postWrapper, as the add-on has to be added before the label. The thing I see now is that the template is loaded from the file, which implies copying the .html file in the project using the library. I think using templateCache could solve the problem, am I right? (Never used it, but saw that is how it works with the other wrappers)

@kentcdodds
Copy link
Member

Yes, you'll need to update the grunt build to look for files from the other directory. Look for wrappers or fields for how that's accomplished. Also, I think that you forgot to add the other directory to the repo. And finally, I think you reformatted the file to use 4 spaces, but this project uses 2 spaces. Please update that. Thanks!

@Ledragon
Copy link
Contributor Author

Oops, my bad for the formatting, I did an auto reformat by reflex, and did not check the configuration.
I'll check how to use template cache and add other folder, and correct the formatting. A poor commit this time :p

@kentcdodds
Copy link
Member

No worries 👍 thanks for the help!

Indentation of 2 spaces isntead of 4
@Ledragon
Copy link
Contributor Author

Hi again,
I commited the changes as per our last exchanges (use of template cache, inclusion of other folder and indenting of 2 spaces).

return template;
}
var tmpl = $templateCache.get('other/formly-other-bootstrap-addons.html');
return tmpl.replace('<formly-transclude></formly-transclude>', template);
Copy link
Member

Choose a reason for hiding this comment

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

This comment has been moved.

@kentcdodds
Copy link
Member

I don't think that I do get notified when you commit changes.

}
]);
}
]);
Copy link
Member

Choose a reason for hiding this comment

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

Sorry to be so nit-picky, but the whitespace around here is all messed up. Please make sure that you're indenting with 2 spaces. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ew you are totally right, thatt's ugly ::shame::
Wil fix this tomorrow

@kentcdodds
Copy link
Member

I think it's looking good. I'll take a little time later to pull it down, test it out, run the build, increment the version, and publish the new version. Thanks!

@Ledragon
Copy link
Contributor Author

Thanks to you for your help and advice, let me know how it goes. I created an example to test it, I can push it to my git if that is of interest, or create a jsbin if formly is available through cdn?

@kentcdodds
Copy link
Member

Would totally love a test. Feel free to make it part of this PR. And for the jsbin example, that would be awesome as well. Please go to the website and click on "Suggest an example"

@Ledragon
Copy link
Contributor Author

I corrected the indentation.
For the example, shall I create a new folder or place it under test (sth like example.html)?

@kentcdodds
Copy link
Member

Thanks. For the example, please go to the website and click on "Suggest an example"

@Ledragon
Copy link
Contributor Author

Great, I'm on it!

@kentcdodds
Copy link
Member

@Ledragon, I'm working on this now. I have a few other things I want to do for the next release. Don't worry, your work wont go to waste :-)

@Ledragon
Copy link
Contributor Author

Thanks for the feedback!

@kentcdodds
Copy link
Member

I'm not sure why this didn't show it as merged, but I've merged your PR and added a bunch of stuff in 4.0.0 :-D Thanks for your contribution!

@kentcdodds kentcdodds closed this Mar 2, 2015
@Ledragon
Copy link
Contributor Author

Ledragon commented Mar 2, 2015

http://www.quickmeme.com/meme/3uwe8u
Most welcome, my pleasure! Now I'm thinking about using control feedback icons, is it something that might be if interest? Shall I open a new issue?

@kentcdodds
Copy link
Member

I'm not familiar with feedback icons, but it definitely belongs in another issue.

@Ledragon
Copy link
Contributor Author

Ledragon commented Mar 2, 2015

I will do that later then, together with an example so that you can see what I mean and decide whether it's interesting for this repo or not!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants