Last modified by Djebloun Sidali on 2021/03/17 21:38

  • Ecaterina Moraru (Valica)
    Ecaterina Moraru (Valica), 2014/01/17 14:04

    Hi Sidali, 

    Interesting concept emoticon_smile Some feedback:
    - ThemesCode space pages should be hidden, same for pages inside Themes space (the generated pages);
    - Themes should have an entry in the Applications panel, read more at http://contrib.xwiki.org/xwiki/bin/view/Main/WebHome#HApplicationDesign
    - Your application pages are orphans, run xar:format on pages to check them for such cases
    - You have problems with the translations, "xwiki.themes.createTheme" does not find the key (tested for english)
    - The picker is not working, tested with IE9, Chrome 31 and Firefox 26
    - You could have used the icon from the default Silk icon set

    For me is not clear what exactly this application is supposed to do. You mentioned something about a Calendar application, but you don't provide a link to what Calendar application you are referring too. Also maybe this dependency should be marked somewhere.

    Thanks emoticon_smile

    • Djebloun Sidali
      Djebloun Sidali, 2014/01/20 09:50

      Thanks for your remarks Ecatrina, this is my first contribution, they helped me. 

       - done for points 1, 3, 4.6 

       - For point 2: Not necessarily because it is not an application in itself, but must be integrated with another application that it should appear in the panel. Themes in general should be handled in the Admin section 

       - For point 5: Colorpiker it works very well for me for three browsers mentioned, I tested with two xwiki versions 4.5.2 and 5.2

       - For the calendar this is not an app that exists, but just an example of use, I will adapt the description.

      But the calendar is under construction, he will come...

    • Djebloun Sidali
      Djebloun Sidali, 2014/01/20 13:16

      Indeed there was a bug for the color picker, I had put the wrong js docs for colorpicker.
      it is fixed now.

  • Ecaterina Moraru (Valica)
    Ecaterina Moraru (Valica), 2014/01/17 14:12

    I'm mentioning the Calendar application, because I've tested with http://extensions.xwiki.org/xwiki/bin/view/Extension/Calendar+Application and is not working.

  • Vincent Massol
    Vincent Massol, 2014/01/17 16:57

    Is this about XWiki's ColorThemes? I don't fully understand this application's desciption, could you elaborate? Thanks!

    • Djebloun Sidali
      Djebloun Sidali, 2014/01/20 09:57

      Is this about XWiki's ColorThemes?
       > No, it's just a new small, simple application that allows you to create objects with two properties 'title' and 'color'. 

      I don't fully understand this application's desciption, could you elaborate? Thanks!
       > ok 

    • Ecaterina Moraru (Valica)
      Ecaterina Moraru (Valica), 2014/01/20 13:45

      How about 'Category Manager' or 'Label Manager' as name for the application? 

  • Anca Luca
    Anca Luca, 2014/01/20 15:06

    Hello Sidali, I looked at the code of the app and I have a few remarks:

    • I saw that there is a page called PrototypeColorPicker. Have you tried to use the colorpicker widget already existing in xwiki (resources/uicomponents/widgets/colorpicker/) ? This is the one used by the standard colorthemes editor UI, and i remember I reused it in an application where I needed to pick colors and it was magically easy.
    • I see a lot of pages with names like Theme_<random string>_<random numbers>, mostly in Themes space and one in ThemeCode space. Are these "content" pages? If so, they should not be in the application, they will be created by the user once they use the installed application.
    • Instead of the GenerateNewThemeDoc script I would have tried to go through the "create" action, to which you would pass the already generated random string. The "create" action is what you get when you click on the Create -> Page menu. The trick is that you send parameters to this action as if they would come from the submitted creation form, and then it would behave just as any manual create would (redirect to inline mode). This allows you to save some code, but more importantly allows to have consistency for the page creation (all creation of docs go through create action).
    • Again for GenerateNewThemeDoc, I don't really understand why you cannot generate the name of the document directly in ThemesSheet, instead of going through another script.
    • building manually document names by concatenating with strings and separating them with . is strongly discouraged . Please use $services.model service to create document references and obtain their string serialization ( Model Module ).
    • I would have named the class differently, to something that contains color as well somewhere, to differentiate it a bit more from the classification application ( Classification Application ), but that is just a preference of mine, you can ignore this comment if you want.
    • If I didn't understand wrong, the CSS in ThemesCode.SSX is specific to the ThemesSheet UI. If so, I would have put the CSS directly in ThemesSheet and used it from there, for cohesion reasons.
  • Vincent Massol
    Vincent Massol, 2014/01/21 08:19

    I've fixed the download URL property value which was wrongly set to http://extensions.xwiki.org/xwiki/bin/download/Extension/Themes+Application/Themes.xar. It's a very bad practice to use full URLs when referencing pages or attachments in the same wiki since it means if you change the spage, space or name of wiki the link will get broken as it was when I renamed the page...

    Also your XAR must have a version, like themes-1.0.xar, especially since you said it's version 1.0. And when you release 1.1 or 2.0 you'll name it themes-1.1.xar or themes-2.0.xar

    Also the name could be improved to be in line with the name of this extension which is cqlled Label Manager. I'd use a name like application-label-manager-1.0.xar.

    Thanks!

  • Djebloun Sidali
    Djebloun Sidali, 2014/01/23 14:45

    This first contirb allowed me to learn many things that I should not do in the future.. thank for your comments 

    @Anca :

    • Done for points 1, 2, 5, 7
    • I will try to use point 3 in the future 
    • For point 4 : i can't for this reaison :
           When user need to create multiple items, by opening multiple borwser tabs, when they click more than one time in the link, they will be redirected to the same doc/form, while he want to have diffrent documents/forms 
    • For point 7 : it was intended to all the application, there are new things that will be added, but ok to put it in the SteelSheet, for the moment

    @ vincent : the xar was renamed

Get Connected