Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0, 2.1, 2.2
    • Fix Version/s: 2.1.3
    • Component/s: Themes
    • Labels:
    • Rank:
      32742

      Description

      The original patches in 7 commits are given in the URL in this page.
      This sub-task is related to MDL-29226

        Issue Links

          Activity

          Hide
          Nadav Kavalerchik added a comment -

          I rebased my local tree and i think there are some missed patches (considering the original: MDL-29226)
          so here are some more, up to date, commits:
          https://github.com/nadavkav/moodle/compare/master...wip-MDL-29226-master-rtl-patches

          Please test them and see if you can integrate them with this issue's commits. (as new ones? maybe?)

          Nadav

          Show
          Nadav Kavalerchik added a comment - I rebased my local tree and i think there are some missed patches (considering the original: MDL-29226 ) so here are some more, up to date, commits: https://github.com/nadavkav/moodle/compare/master...wip-MDL-29226-master-rtl-patches Please test them and see if you can integrate them with this issue's commits. (as new ones? maybe?) Nadav
          Hide
          Mary Evans added a comment - - edited

          Hi Nadav,

          Nothing is missing, it is just that they are divided in to two sub-tasks

          MDL-29226 - contains 2 of your original commits
          MDL-30359 - contains 5 of your original commits

          I only did what Sam suggested to you in another in RTL (take 2)

          I can add you link to the github repo in the description, then if they need to track it they can. OK?
          Mary

          Show
          Mary Evans added a comment - - edited Hi Nadav, Nothing is missing, it is just that they are divided in to two sub-tasks MDL-29226 - contains 2 of your original commits MDL-30359 - contains 5 of your original commits I only did what Sam suggested to you in another in RTL (take 2) I can add you link to the github repo in the description, then if they need to track it they can. OK? Mary
          Hide
          Nadav Kavalerchik added a comment -

          You are right. my mistake.

          btw, I continue to add new patches to a new branch: wip-MDL-30359-master-rtlpatches

          I have added some more patches, today.
          Here is a link: https://github.com/nadavkav/moodle/compare/master...wip-MDL-30359-master-rtlpatches

          I have more. and i keep updating this branch.
          What should i do with them?

          Show
          Nadav Kavalerchik added a comment - You are right. my mistake. btw, I continue to add new patches to a new branch: wip- MDL-30359 -master-rtlpatches I have added some more patches, today. Here is a link: https://github.com/nadavkav/moodle/compare/master...wip-MDL-30359-master-rtlpatches I have more. and i keep updating this branch. What should i do with them?
          Hide
          Mary Evans added a comment - - edited

          I'm confused now.

          Why didn't you create a new sub-task in MDL-30337 which is what I was trying to do before you panicked me into adding those other commits back under MDL-29226.

          That way you could have added these latest commits to the new issue number, separate from MDL-29226 and MDL-30359. You can't add them to MDL-30359 because it is integration.

          Also is it necessary to mark each change you do? Can you not just make all the changes in whatever file you need to make the changes in, like theme/base/style/core.css? What I mean is do all the changes, add them to your new branch and submit them in one commit message something like "MDL-xxxxx-master - RTL fixes for base/core css" or something similar?

          Thinking about this...and the many css fixes you are doing which is absolutely fantastic, as I don't think anyone really understands the amount of changes needed, and that in retrospect it would have been better adding them to a separate file as used in Moodle 1.9.

          Having a rtl.css file would make your life a lot easier as far as the css is concerned. This way you would be able to add various commented sections under which you could add the css. Making it easier to trace.

          So before you send any more commits with those wrong MDL prefixes please create a new sub-task in MDL-30337 and add your (renamed) commits to it.

          I hope this is OK with you?

          Perhaps Eloy or Sam...who do quite a lot of the integrating for the themes...and better able to offer their advice as I am really only the office junior! LOL

          Cheers
          Mary

          Show
          Mary Evans added a comment - - edited I'm confused now. Why didn't you create a new sub-task in MDL-30337 which is what I was trying to do before you panicked me into adding those other commits back under MDL-29226 . That way you could have added these latest commits to the new issue number, separate from MDL-29226 and MDL-30359 . You can't add them to MDL-30359 because it is integration. Also is it necessary to mark each change you do? Can you not just make all the changes in whatever file you need to make the changes in, like theme/base/style/core.css? What I mean is do all the changes, add them to your new branch and submit them in one commit message something like "MDL-xxxxx-master - RTL fixes for base/core css" or something similar? Thinking about this...and the many css fixes you are doing which is absolutely fantastic, as I don't think anyone really understands the amount of changes needed, and that in retrospect it would have been better adding them to a separate file as used in Moodle 1.9. Having a rtl.css file would make your life a lot easier as far as the css is concerned. This way you would be able to add various commented sections under which you could add the css. Making it easier to trace. So before you send any more commits with those wrong MDL prefixes please create a new sub-task in MDL-30337 and add your (renamed) commits to it. I hope this is OK with you? Perhaps Eloy or Sam...who do quite a lot of the integrating for the themes...and better able to offer their advice as I am really only the office junior! LOL Cheers Mary
          Hide
          Nadav Kavalerchik added a comment -

          I started getting confused too with all the procedures, statues, stages, MDLs, tasks and sub tasks...
          I guess it is all very logic to you (Moodle HQ) but i was not sure any more what do i need to do to get it
          integrated on time (before 2.2)

          Let's sort it out, after Moodle 2.2 is out.

          The name i gave my local git branch, is just a name. It will be changed as soon as i start a new MDL issue.
          That is way i asked you "What to do" with these new suggested patches.

          I opened a new sub task, as you suggested : MDL-30361
          And soon fix this misunderstanding

          Anyways...
          I do not mind adding those RTL patches along side the regular core CSS rules.
          It is not confusing me and I am not sure if any one cares what each CSS role does.( Am i right? )

          If you'd like to make a rtl.css file for base or standard, that's ok too.

          Nadav

          Show
          Nadav Kavalerchik added a comment - I started getting confused too with all the procedures, statues, stages, MDLs, tasks and sub tasks... I guess it is all very logic to you (Moodle HQ) but i was not sure any more what do i need to do to get it integrated on time (before 2.2) Let's sort it out, after Moodle 2.2 is out. The name i gave my local git branch, is just a name. It will be changed as soon as i start a new MDL issue. That is way i asked you "What to do" with these new suggested patches. I opened a new sub task, as you suggested : MDL-30361 And soon fix this misunderstanding Anyways... I do not mind adding those RTL patches along side the regular core CSS rules. It is not confusing me and I am not sure if any one cares what each CSS role does.( Am i right? ) If you'd like to make a rtl.css file for base or standard, that's ok too. Nadav
          Hide
          Nadav Kavalerchik added a comment -

          One more thing...

          referring to:
          "Also is it necessary to mark each change you do? Can you not just make all the changes in whatever file you need to make the changes in, like theme/base/style/core.css? What I mean is do all the changes, add them to your new branch and submit them in one commit message something like "MDL-xxxxx-master - RTL fixes for base/core css" or something similar?"

          When we QA the UI for RTL issues, we run through the Modules, Blocks, Menus... and not by going linearly through the files on the theme. so it is not so easy to find all the necessary changes that should go into (let's say) core.css and commit them as one patch.

          Also, I try to commit on every major UI change i make. So it will be easier for your team to understand what that single change was about. It is much easier for me the squash a list of changes into one commit, if you like.

          If you do not care about the information (UI issue description) that goes with each line of CSS i change. i can make a list of changes, hold them back, locally, and then commit them, once every two weeks? (maybe) with a commit message something like "MDL-xxxxx-master - RTL fixes for base/core css" an so on, for each file that get changed.
          Is it more convenient for you to check and integrate this way?

          Nadav

          Show
          Nadav Kavalerchik added a comment - One more thing... referring to: "Also is it necessary to mark each change you do? Can you not just make all the changes in whatever file you need to make the changes in, like theme/base/style/core.css? What I mean is do all the changes, add them to your new branch and submit them in one commit message something like "MDL-xxxxx-master - RTL fixes for base/core css" or something similar?" When we QA the UI for RTL issues, we run through the Modules, Blocks, Menus... and not by going linearly through the files on the theme. so it is not so easy to find all the necessary changes that should go into (let's say) core.css and commit them as one patch. Also, I try to commit on every major UI change i make. So it will be easier for your team to understand what that single change was about. It is much easier for me the squash a list of changes into one commit, if you like. If you do not care about the information (UI issue description) that goes with each line of CSS i change. i can make a list of changes, hold them back, locally, and then commit them, once every two weeks? (maybe) with a commit message something like "MDL-xxxxx-master - RTL fixes for base/core css" an so on, for each file that get changed. Is it more convenient for you to check and integrate this way? Nadav
          Hide
          Mary Evans added a comment -

          Let's get this straight...I only started learning about GIT in Feb/March this year, and I am still learning, so don't take what I say as gospel truth...as I could be wrong. You probably know more than me anyway!

          You carry on the way you do your commits that way you will know where you are up to in all these changes. Perhaps it may be possible for you to be able to submit these for peer review/integration yourself in the future. But that would be up to Martin.

          If you are willing to add these every couple of weeks that is OK by me.

          Thanks
          Mary

          Show
          Mary Evans added a comment - Let's get this straight...I only started learning about GIT in Feb/March this year, and I am still learning, so don't take what I say as gospel truth...as I could be wrong. You probably know more than me anyway! You carry on the way you do your commits that way you will know where you are up to in all these changes. Perhaps it may be possible for you to be able to submit these for peer review/integration yourself in the future. But that would be up to Martin. If you are willing to add these every couple of weeks that is OK by me. Thanks Mary
          Hide
          Nadav Kavalerchik added a comment -

          Great

          Show
          Nadav Kavalerchik added a comment - Great
          Hide
          Sam Hemelryk added a comment -

          Thank Mary - these changes have been integrated now

          Show
          Sam Hemelryk added a comment - Thank Mary - these changes have been integrated now
          Hide
          Sam Hemelryk added a comment -

          Passing this now.
          I did have to make an additional commit to fix a regression with the course completion icons (right and left were the wrong way round)

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Passing this now. I did have to make an additional commit to fix a regression with the course completion icons (right and left were the wrong way round) Cheers Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Closing as fixed, many thanks for your effort!

          Note that the changes related to master (2.2beta) have been already sent upstream. But the stable ones will be part of next weeklies (Wed/Thu) as usual.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Closing as fixed, many thanks for your effort! Note that the changes related to master (2.2beta) have been already sent upstream. But the stable ones will be part of next weeklies (Wed/Thu) as usual. Ciao
          Hide
          Mary Evans added a comment -

          Thank Eloy!

          Show
          Mary Evans added a comment - Thank Eloy!

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: