This application is not used any more and exists only for history. Post new tickets on the Github account.
Cette application n'est plus utilisée, et existe uniquement pour son historique. Postez les nouveaux tickets sur le compte github.

Ticket #922 (assigned enhancement)

Opened 5 years ago

Last modified 2 years ago

pagelinks plugin uses offsets instead of page numbers

Reported by: Surfoo Owned by: laurentj
Priority: low Milestone:
Component: jelix:plugins:tpl Version: 1.1.2
Severity: normal Keywords:
Cc: Blocked By:
Blocking: Documentation needed: no
Hosting Provider: Php version:

Description

Currently, pagelinks uses offsets but page numbers would be better for everyone, it's more understandable !

Attachments

922-pagelinks.diff (8.3 KB) - added by Surfoo 5 years ago.
922-pageslinks-2.diff (8.2 KB) - added by Surfoo 5 years ago.

Change History

Changed 5 years ago by Surfoo

comment:1 Changed 5 years ago by Surfoo

  • review set to review?

So this is my patch for fix it and i changed labels too, it's less ugly, isn't it ?

comment:2 Changed 5 years ago by Surfoo

  • Summary changed from pagelinks plugon uses offsets instead of page numbers to pagelinks plugin uses offsets instead of page numbers

comment:3 Changed 5 years ago by laurentj

  • review changed from review? to review-

The problem of your patch is that you break all existing use of this plugin, isn't it ?

Why did you replace "," by "." in all echo instructions ? It is better to use a ",", because there isn't a concatenation...

comment:4 Changed 5 years ago by bballizlife

We're facing a little problem here as your path break compatibility with existing uses, as said by laurentj. On the other hand, i agree that using pages instead of offset is a better way. As of now, i don't know how to deal with this issue. Maybe a "pagelinkspages" plugin ?

At last, i don't mind about the "s/,/."

comment:5 Changed 5 years ago by bballizlife

Well, i thought about this issue.

  • Do we want a "pagelinks" plugin that works with pages instead of offset ?

I'd like to.

  • Can we break compatibility ?

I think we cannot as this will force to much code changes for the users (computing pages numbers, modifying their URLs settings, ...)

  • Can we add the "pages feature" and keep compatibility ?

This will need to modify the prototype of the function. Not impossible but i think such a way will add to much complexity to the function. It already has 7 parameters, that's enough. We need to think about the ease of use for developers

  • So what can we do ?

My conclusion is that the simplest way is to code another plugin we could name : "pagelinks_pages" for example.

The only thing we must take care about is both "pagelinks" plugin should generate the same html (tags and attributes).

What about you ?

comment:6 Changed 5 years ago by laurentj

  • review review- deleted

I propose to replace the $paramName parameter by an array of options. allowed values in this array are:

  'paramName'=>'offset',
  'behavior'=>'page' or 'offset'

If you find a better name for 'behavior'...

The plugin should check the type of this parameter. If this is a string, 'paramName'=> the value of the parameter, and 'behavior'=>'offset'. If this is an array, try to get 'paramName' and 'behavior' from into it. If the 'behavior' key in this array is missing, default value is 'page'.

comment:7 Changed 5 years ago by bballizlife

Nice idea. I agree with it.

@Surfoo : could you update your patch with what laurentj said ?

Changed 5 years ago by Surfoo

comment:8 Changed 5 years ago by Surfoo

  • review set to review?

Désolé je la fait en français là..

J'ai dupliqué le code au lieu de faire des hack partout pour savoir si c'était un comportement d'offset ou de page qui était utilisé. Surtout que c'est toujours des petites modifications (genre un caractère à changer) et avoir le meme style de hack partout, bof.. Qu'est ce que vous en pensez comme ça ?

J'ai aussi supprimé le dernier else en bas du code, ça sert à rien d'avoir une pagination d'une seule page et qui ne soit pas cliquable (et j'ai rajouté un return false pour vérifier ça plus haut).

comment:9 Changed 5 years ago by bballizlife

Not to display pagination if there's only 1 page is ok for me. So as for separating behaviours. It's more readable i think too.

But as unit tests fail, i can't accept the patch for now. I don't think it's a huge tasks so please take care at the UT. Thanks.

Then we should be ok with that patch.

comment:10 Changed 4 years ago by laurentj

  • Milestone changed from Jelix 1.1.4 to Jelix 1.2

comment:11 Changed 4 years ago by laurentj

  • review changed from review? to review-

because of fails of unit tests

comment:12 Changed 4 years ago by laurentj

  • Milestone Jelix 1.2 beta deleted

comment:13 Changed 2 years ago by laurentj

  • Status changed from new to assigned
  • Owner changed from Surfoo to laurentj
  • review review- deleted
Note: See TracTickets for help on using tickets.