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

HTML content opener #9

Closed
wants to merge 2 commits into from
Closed

HTML content opener #9

wants to merge 2 commits into from

Conversation

randoum
Copy link

@randoum randoum commented Sep 26, 2015

Add functionality to use HTML code for the open and close openers content by setting opener.as = 'html', allowing for example to use font-awesome icons by writing options like this :

    options =
      ...
      opener:
        active: true
        as: 'html'
        close: '<i class="fa fa-plus"></i>'
        open: '<i class="fa fa-minus"></i>'
        openerCss:
          'float': 'left'
          'margin-left': '-27px'
          'margin-top': '1px'
          'cursor': 'pointer'

    $('ul#hierarchy-tree').sortableLists options

opener

@camohub
Copy link
Owner

camohub commented Sep 26, 2015

Hi,
It is good idea. But make some refactoring please. Mainly use camelcase in fc and vars names. I did't check it properly yet. Give me a little time.
Have a nice day!

@@ -129,8 +131,10 @@

if ( setting.opener.active )
{
if ( ! setting.opener.open ) throw 'Url for opener.open image is not defined';
if ( ! setting.opener.close ) throw 'Url for opener.close image is not defined';
if ( ! setting.opener.as || (setting.opener.as !== 'img' && setting.opener.as !== 'html') )
Copy link
Owner

Choose a reason for hiding this comment

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

This should be enough
if ( setting.opener.as !== 'img' && setting.opener.as !== 'html' )
sholdn't be?

@randoum
Copy link
Author

randoum commented Sep 29, 2015

Done with your 2 feedbacks, let me know if there is more I can do

@camohub
Copy link
Owner

camohub commented Nov 5, 2015

Hi I have it still on mind. But I am not 100% sure and have not enough time.

@randoum
Copy link
Author

randoum commented Nov 5, 2015

Ok noted.
Know that I use the patch every day and it work flawlessly.
Hope to see it merged soon :)
Cheers

@@ -43,6 +43,7 @@
},
opener: {
active: false,
as: 'img',
Copy link
Owner

Choose a reason for hiding this comment

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

We can leave it without as. Only if user want to switch to html he set it to "html". May be that param could be named as html: true,
Later in code(line 134) I would remove that condition. Next two conditions should be enough.

Copy link
Owner

Choose a reason for hiding this comment

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

Now I see that if we leave it without as and you will check it you must do

If ( o.as && o.as === 'html' ) 

So there are two conditions but I would like to have one
Solution is to have as as default and check

if ( o.as ==== 'html' )

or param will be optional like asHtm: true and so condition will be

If ( o.asHtml ) 

I would like to have that last option.

@randoum
Copy link
Author

randoum commented Nov 6, 2015

Actually I used as 'img' and as 'html' in order to make it easy to add other modes in the future, someone may want to add a css class mode with as 'class'. What do you think?

@camohub
Copy link
Owner

camohub commented Nov 6, 2015

For the future is important to have one place where you easy find and change this implementation. It solves setOpenClose() function. Conditions if ( html ) and if ( as === 'html' ) have the same efect. If you want to add cssClass if ( html ) will be false. No problem. Its easy to extend it in the future.

@camohub
Copy link
Owner

camohub commented Nov 6, 2015

About that future. I think html will change current url and if you have html elements you can add class directly to them.

@camohub
Copy link
Owner

camohub commented Nov 6, 2015

!!!!!! You are right with that opener[state] !!!!!!!! My head is out of order...

@camohub
Copy link
Owner

camohub commented Feb 2, 2016

Thank you for good idea. I made it little different.

@camohub camohub closed this Feb 2, 2016
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

2 participants