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

Added $templateFile to UI\Control #206

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

Conversation

holantomas
Copy link

  • new feature
  • BC break? no (3rd party apps maybe ...?)
  • doc PR: not needed

I made topic for this on forum: https://forum.nette.org/en/31591-ui-control-templatefile

@mabar
Copy link
Contributor

mabar commented Dec 10, 2018

I prefer a higher level abstraction which would use views instead of specific templates, similarly like presenters do.

{
$this->templateFile = $templateFile;

if ($this->template !== null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Conditional setter? Never seen that before. Should throw exception or create template if not already created.
I understand why it's here, but it's a just a very specific usecase.

Copy link
Author

Choose a reason for hiding this comment

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

I don't know if you got it right. Whole idea of this PR is to be able setup file without creating template. If we throw exception or create template there is no PR needed any more, cause it will work just like shortcut for $control->getTemplate()->setFile().

But I thought about it if I should add this condition. Because it's just for specific usecase as you said when user try to set file after template was created to prevent "WTF?" situations and let setter work like user expect.

Copy link
Contributor

@mabar mabar Dec 10, 2018

Choose a reason for hiding this comment

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

As I said, I understand. But imagine you don't know that code, just api. It looks exactly like a shortcut, but acts absolutely different.

Copy link
Author

Choose a reason for hiding this comment

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

I don't, I'm not saying you're not right, but I think user don't care about it, he just want to set template file/view and this did that without unnecessary loads.

And I really hate setting template in $onAnchor event. Or do you know way how to do it without event in control factory or in createComponent<name>() method?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could configure template file through it's factory definition

fooControlFactory:
  implement IFooControlFactory
  setup:
    - getTemplate()->setFile('/path/to/file')

But I never used that approach. I have ITemplate with setView method and to change path to these views I only need change pattern in configuration.

Copy link
Author

Choose a reason for hiding this comment

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

This will end with InvalidStateException or new template instance. Anyway I don't like to setup file template in config, there are situations where you want to setup view by condition, or you have one control with factory and want to setup many different view for actual page.

@holantomas
Copy link
Author

holantomas commented Dec 10, 2018

@mabar I'm OK with that 👍 If more people will consider it as good way I can try to implement it.

@mabar
Copy link
Contributor

mabar commented Dec 10, 2018

I think you cannot as it requires more than ITemplate interface

@milo
Copy link
Member

milo commented Dec 11, 2018

IMHO, template file is internal thing of Control, thing of encapsulation. Control may use more templates too, that's why you usualy set template file in render() method.

Why do you need set template file outside of Control?

@holantomas
Copy link
Author

holantomas commented Dec 11, 2018

IMHO, template file is internal thing of Control, thing of encapsulation. Control may use more templates too, that's why you usualy set template file in render() method.

Why do you need set template file outside of Control?

It's shown in forum topic. User usign 3rd party control like datagrid or paginator but HTML not equal to his UI for example.

encapsulation:
Control::getTemplate(): ITemplate is public
ITemplate::setFile() is public
This doesn't break anything.

And I'm not sure but settings template file in render() is to prevent unnecessary creating template or prevent creating template before Control is attached to presenter I think. Both doing my PR too.

@dg dg force-pushed the master branch 8 times, most recently from 10a5830 to e402814 Compare February 10, 2019 17:46
@dg dg force-pushed the master branch 3 times, most recently from 56c5e11 to 5919611 Compare February 13, 2019 00:53
@dg dg force-pushed the master branch 5 times, most recently from 1617426 to bea304e Compare March 12, 2019 00:51
@dg dg force-pushed the master branch 5 times, most recently from e4610ae to 5a520b1 Compare April 1, 2019 22:11
@dg dg force-pushed the master branch 2 times, most recently from a1e8c9b to df7c4c9 Compare January 21, 2024 23:50
@dg dg force-pushed the master branch 2 times, most recently from 891b6dd to e3d05b3 Compare February 2, 2024 17:51
@dg dg force-pushed the master branch 3 times, most recently from 426e735 to c19ebdc Compare March 11, 2024 20:02
@dg dg force-pushed the master branch 5 times, most recently from 2b9da37 to 30d90f4 Compare April 7, 2024 02:51
@dg dg force-pushed the master branch 6 times, most recently from bf86204 to c91f90a Compare April 20, 2024 00:46
@dg dg force-pushed the master branch 3 times, most recently from 57bd587 to e908315 Compare May 2, 2024 10:37
@dg dg force-pushed the master branch 8 times, most recently from c5ecbda to ecb200c Compare May 13, 2024 09:25
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.

None yet

3 participants