Uploaded image for project: 'OFBiz'
  1. OFBiz
  2. OFBIZ-7311

Formatting and renaming of the CSS files as per best practices

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: Trunk
    • Fix Version/s: 16.11.01
    • Component/s: themes
    • Labels:
      None
    • Sprint:
      Community Day 2 - 2016

      Description

      In CSS files, we are having various inconsistency in the formatting of the code for e.g. we have following types of code formatting

      // No space

      ul#preferences-menu a:hover {
      text-decoration: none;
      }
      

      // Space of 2

      div.autocomplete ul {
        list-style-type:none;
        margin:0;
        padding:0;
      }
      

      // Space of 4

      .control-area a {
          font-size: 1.1em;
          color: #5CA3D7;
      }
      

      For better readability we should format the existing files with space of 4.

      Also for some theme, we named CSS files as maincss.css and for some, we named it as style.css. We can rename these file as per best practices for consistency.

      1. OFBIZ-7311-bizznesstime.patch
        145 kB
        Swapnil M Mane
      2. OFBIZ-7311-bluelight.patch
        118 kB
        Swapnil M Mane
      3. OFBIZ-7311-droppingcrumbs.patch
        144 kB
        Swapnil M Mane
      4. OFBIZ-7311-flatgrey.patch
        152 kB
        Swapnil M Mane
      5. OFBIZ-7311-multiflex.patch
        136 kB
        Swapnil M Mane
      6. OFBIZ-7311-rainbowstone.patch
        97 kB
        Swapnil M Mane
      7. OFBIZ-7311-removed_references_of_maincss.patch
        37 kB
        Swapnil M Mane
      8. OFBIZ-7311-renamed-maincss-to-style.patch
        14 kB
        Swapnil M Mane
      9. OFBIZ-7311-tomahawk.patch
        176 kB
        Swapnil M Mane

        Activity

        Hide
        swapnilmmane Swapnil M Mane added a comment -

        Attaching the patches with respect to each theme for the formatted code.

        Show
        swapnilmmane Swapnil M Mane added a comment - Attaching the patches with respect to each theme for the formatted code.
        Hide
        swapnilmmane Swapnil M Mane added a comment -

        To maintain the consistency, renamed the maincss.css to style.css (also mainrtl.css to stylertl.css)

        This patch is to update the references of files after renaming.
        After applying this patch, kindly run the following commands from OFBiz home

        svn mv themes/flatgrey/webapp/flatgrey/maincss.css themes/flatgrey/webapp/flatgrey/style.css
        svn mv themes/flatgrey/webapp/flatgrey/mainrtl.css themes/flatgrey/webapp/flatgrey/stylertl.css

        svn mv themes/rainbowstone/webapp/rainbowstone/maincss.css themes/rainbowstone/webapp/rainbowstone/style.css
        svn mv themes/rainbowstone/webapp/rainbowstone/mainrtl.css themes/rainbowstone/webapp/rainbowstone/stylertl.css

        Show
        swapnilmmane Swapnil M Mane added a comment - To maintain the consistency, renamed the maincss.css to style.css (also mainrtl.css to stylertl.css ) This patch is to update the references of files after renaming. After applying this patch, kindly run the following commands from OFBiz home svn mv themes/flatgrey/webapp/flatgrey/maincss.css themes/flatgrey/webapp/flatgrey/style.css svn mv themes/flatgrey/webapp/flatgrey/mainrtl.css themes/flatgrey/webapp/flatgrey/stylertl.css svn mv themes/rainbowstone/webapp/rainbowstone/maincss.css themes/rainbowstone/webapp/rainbowstone/style.css svn mv themes/rainbowstone/webapp/rainbowstone/mainrtl.css themes/rainbowstone/webapp/rainbowstone/stylertl.css
        Hide
        pandeypranay Pranay Pandey added a comment -

        Thanks so much @Swapnil M Mane for this contribution towards the betterment of overall readability of CSS files in all the themes.
        I request all the developers working on OFBiz Trunk to do ./ant clean-all load-demo after updating code post this revision as renamed CSS references have been updated in theme configuration.

        Patches committed to trunk revisions- 1748916, 1748917, 1748918, 1748919, 1748920, 1748921, 1748922, 1748923, 1748924.

        Show
        pandeypranay Pranay Pandey added a comment - Thanks so much @Swapnil M Mane for this contribution towards the betterment of overall readability of CSS files in all the themes. I request all the developers working on OFBiz Trunk to do ./ant clean-all load-demo after updating code post this revision as renamed CSS references have been updated in theme configuration. Patches committed to trunk revisions- 1748916, 1748917, 1748918, 1748919, 1748920, 1748921, 1748922, 1748923, 1748924.
        Hide
        wt Wai added a comment -

        There is some cleanup required.
        when the css file name changed to use style.css and stylertl.css, there are still references to maincss.css and mainrtl.css thoughout the source code.

        Show
        wt Wai added a comment - There is some cleanup required. when the css file name changed to use style.css and stylertl.css, there are still references to maincss.css and mainrtl.css thoughout the source code.
        Hide
        pfm.smits Pierre Smits added a comment -

        In the subject you refer to 'best practices'. Please reference the OFBiz wiki page that outline the best practices for CSS files for completeness.

        Show
        pfm.smits Pierre Smits added a comment - In the subject you refer to 'best practices'. Please reference the OFBiz wiki page that outline the best practices for CSS files for completeness.
        Hide
        pandeypranay Pranay Pandey added a comment -

        Yes I agree Pierre, here is the link to the document from OFBiz wiki https://cwiki.apache.org/confluence/x/B4B2.

        Show
        pandeypranay Pranay Pandey added a comment - Yes I agree Pierre, here is the link to the document from OFBiz wiki https://cwiki.apache.org/confluence/x/B4B2 .
        Hide
        pandeypranay Pranay Pandey added a comment -

        Thanks Wai for reviewing the commit and posting your finding. Yes I completely agree with you. As with these patches all themes were tested and found working fine I closed this ticket. Reopening it now, to remove the remaining references to maincss.css and mainrtl.css. As of now there are total 50 occurrences found for maincss.css and 1 occurrence found for mainrtl.css.

        Show
        pandeypranay Pranay Pandey added a comment - Thanks Wai for reviewing the commit and posting your finding. Yes I completely agree with you. As with these patches all themes were tested and found working fine I closed this ticket. Reopening it now, to remove the remaining references to maincss.css and mainrtl.css. As of now there are total 50 occurrences found for maincss.css and 1 occurrence found for mainrtl.css.
        Hide
        pandeypranay Pranay Pandey added a comment -

        Reopening the ticket to check and correct/clear usage of maincss.css and mainrtl.css based on Wai's comment.

        Show
        pandeypranay Pranay Pandey added a comment - Reopening the ticket to check and correct/clear usage of maincss.css and mainrtl.css based on Wai's comment.
        Hide
        swapnilmmane Swapnil M Mane added a comment -

        Thanks so much Pranay Pandey

        Show
        swapnilmmane Swapnil M Mane added a comment - Thanks so much Pranay Pandey
        Hide
        swapnilmmane Swapnil M Mane added a comment -

        Thanks Wai, Pierre Smits and Pranay Pandey for the discussion.
        I will have the look into the remaining references of maincss.css and mainrtl.css in source code and will update them accordingly.

        Show
        swapnilmmane Swapnil M Mane added a comment - Thanks Wai , Pierre Smits and Pranay Pandey for the discussion. I will have the look into the remaining references of maincss.css and mainrtl.css in source code and will update them accordingly.
        Hide
        pfm.smits Pierre Smits added a comment -

        I suggest to update https://cwiki.apache.org/confluence/display/OFBADMIN/HTML+and+CSS+Best+Practices so that it better reflects the changes...

        Show
        pfm.smits Pierre Smits added a comment - I suggest to update https://cwiki.apache.org/confluence/display/OFBADMIN/HTML+and+CSS+Best+Practices so that it better reflects the changes...
        Hide
        swapnilmmane Swapnil M Mane added a comment -

        Hello team,

        I have removed the unused references of the maincss.css from source code, done the following things

        1.) Removed 'maincss.css' entry from 'allowedPaths' param of from web.xml
        Reason: Since, there is no file like 'maincss.css' exist under any includes directory.

        2.) Removed the unused references of maincss.css from FTLs, code

        <link rel="stylesheet" href="${StringUtil.wrapString(baseUrl!)}/images/maincss.css" type="text/css"/>
        

        has been removed.

        Reason: This is an interesting thing, these FTLs were using the maincss.css located under the images directory.
        When I look the history of these files, following were my findings,

        The 'maincss.css' file under the 'images' directory exist in release4.0
        http://svn.apache.org/repos/asf/ofbiz/branches/release4.0/framework/images/webapp/images/

        after this release, this files no longer exist there.

        3.) Also, I have updated one file 'InventoryNoticeEmail.ftl' and used the basic HTML code to style, instead of using style defined in 'maincss.css' which were exist in release-4.0

        4.) Updated some references from 'maincss.css' to style.css in comments.

        5.) Also removed the entry of including '/images/maincss.css' and '/images/mainrtl.css' in SimpleDecorator
        Since, these files no longer exist after release-4.0.

        Dear Pranay Pandey, Wai and team,
        Please have a look this patch

        Thanks!

        Show
        swapnilmmane Swapnil M Mane added a comment - Hello team, I have removed the unused references of the maincss.css from source code, done the following things 1.) Removed 'maincss.css' entry from 'allowedPaths' param of from web.xml Reason: Since, there is no file like 'maincss.css' exist under any includes directory. 2.) Removed the unused references of maincss.css from FTLs, code <link rel= "stylesheet" href= "${StringUtil.wrapString(baseUrl!)}/images/maincss.css" type= "text/css" /> has been removed. Reason: This is an interesting thing, these FTLs were using the maincss.css located under the images directory. When I look the history of these files, following were my findings, The 'maincss.css' file under the 'images' directory exist in release4.0 http://svn.apache.org/repos/asf/ofbiz/branches/release4.0/framework/images/webapp/images/ after this release, this files no longer exist there. 3.) Also, I have updated one file 'InventoryNoticeEmail.ftl' and used the basic HTML code to style, instead of using style defined in 'maincss.css' which were exist in release-4.0 4.) Updated some references from 'maincss.css' to style.css in comments. 5.) Also removed the entry of including '/images/maincss.css' and '/images/mainrtl.css' in SimpleDecorator Since, these files no longer exist after release-4.0. Dear Pranay Pandey , Wai and team, Please have a look this patch Thanks!
        Hide
        wt Wai added a comment -

        There are still references to maincss.css and mainrtl.css. These files do not exist anymore.

        Show
        wt Wai added a comment - There are still references to maincss.css and mainrtl.css. These files do not exist anymore.
        Hide
        swapnilmmane Swapnil M Mane added a comment -

        Dear Wai,

        It seems you have missed the recent patch uploaded on 15/Jul/16 15:36, OFBIZ-7311-removed_references_of_maincss.patch

        https://issues.apache.org/jira/secure/attachment/12818181/OFBIZ-7311-removed_references_of_maincss.patch

        Please let me know, if I am still missing something.

        Thanks for your inputs.

        Swapnil M Mane

        Show
        swapnilmmane Swapnil M Mane added a comment - Dear Wai , It seems you have missed the recent patch uploaded on 15/Jul/16 15:36, OFBIZ-7311 -removed_references_of_maincss.patch https://issues.apache.org/jira/secure/attachment/12818181/OFBIZ-7311-removed_references_of_maincss.patch Please let me know, if I am still missing something. Thanks for your inputs. Swapnil M Mane
        Hide
        wt Wai added a comment -

        Thanks, I thought your changes were already applied to the trunk.

        Show
        wt Wai added a comment - Thanks, I thought your changes were already applied to the trunk.
        Hide
        swapnilmmane Swapnil M Mane added a comment -

        Show
        swapnilmmane Swapnil M Mane added a comment -
        Hide
        pandeypranay Pranay Pandey added a comment -

        Committed patch OFBIZ-7311-removed_references_of_maincss.patch to trunk r1758389. Thanks Wai for reviewing the work and thanks Swapnil M Mane for the contribution.

        Show
        pandeypranay Pranay Pandey added a comment - Committed patch OFBIZ-7311 -removed_references_of_maincss.patch to trunk r1758389. Thanks Wai for reviewing the work and thanks Swapnil M Mane for the contribution.
        Hide
        swapnilmmane Swapnil M Mane added a comment -

        Thanks Pranay Pandey!

        Show
        swapnilmmane Swapnil M Mane added a comment - Thanks Pranay Pandey !

          People

          • Assignee:
            pandeypranay Pranay Pandey
            Reporter:
            swapnilmmane Swapnil M Mane
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development

                Agile