Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-40786

User's description fields should be aligned to the right and spaced out, when in RTL mode (theme/clean, bootstrapbase)

    Details

    • Story Points (Obsolete):
      8
    • Sprint:
      FRONTEND Sprint 8

      Description

      User's description fields should be aligned to the right and spaced out, when in RTL mode (theme/clean, bootstrapbase)

      Labels should be on the right side and user's data should be on the left side.
      Overall, they should be right aligned. (On all screen sizes)

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Sorry, have to reopen this one too. It's conflicting (like MDL-39682). Please resolve conflicts once current weekly lands and send it back to integration.

              TIA and ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Sorry, have to reopen this one too. It's conflicting (like MDL-39682 ). Please resolve conflicts once current weekly lands and send it back to integration. TIA and ciao
              Hide
              nadavkav Nadav Kavalerchik added a comment -

              Rebased (24/7/2013)

              Show
              nadavkav Nadav Kavalerchik added a comment - Rebased (24/7/2013)
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              LOL, same than https://tracker.moodle.org/browse/MDL-39682#comment-234848

              Reopening... thanks for your effort!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - LOL, same than https://tracker.moodle.org/browse/MDL-39682#comment-234848 Reopening... thanks for your effort!
              Hide
              nadavkav Nadav Kavalerchik added a comment -

              Indeed I was too quick to rebase LOL

              I have just now rebased both branches onto latest (16/8/2013) master and MOODLE_25_STABLE.
              Ready for peer-review

              Show
              nadavkav Nadav Kavalerchik added a comment - Indeed I was too quick to rebase LOL I have just now rebased both branches onto latest (16/8/2013) master and MOODLE_25_STABLE. Ready for peer-review
              Hide
              lazydaisy Mary Evans added a comment -

              Looks OK so pushing to integration review

              Show
              lazydaisy Mary Evans added a comment - Looks OK so pushing to integration review
              Hide
              marina Marina Glancy added a comment -

              Hi Nadav,
              thanks for you constant work on moodle RTL issues!

              There is a little thing that needs more consideration in your patch:
              https://github.com/nadavkav/moodle/compare/MDL-40786_master#L0R144

              First, it should be 100px and not just "100". Second, it breaks how all forms look. Please resubmit when ready. Thanks!

              Show
              marina Marina Glancy added a comment - Hi Nadav, thanks for you constant work on moodle RTL issues! There is a little thing that needs more consideration in your patch: https://github.com/nadavkav/moodle/compare/MDL-40786_master#L0R144 First, it should be 100px and not just "100". Second, it breaks how all forms look. Please resubmit when ready. Thanks!
              Hide
              cibot CiBoT added a comment -

              Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

              Show
              cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
              Hide
              moodle.com moodle.com added a comment -

              As with MDL-40752, the consistency of this change should be checked across all themes.

              Show
              moodle.com moodle.com added a comment - As with MDL-40752 , the consistency of this change should be checked across all themes.
              Hide
              phalacee Jason Fowler added a comment -

              Taking over from Nadav to get this through.

              Show
              phalacee Jason Fowler added a comment - Taking over from Nadav to get this through.
              Hide
              phalacee Jason Fowler added a comment -

              The other themes don't need this. just clean, because MDL-42487 fixed the other themes it would seem.

              Show
              phalacee Jason Fowler added a comment - The other themes don't need this. just clean, because MDL-42487 fixed the other themes it would seem.
              Hide
              fred Frédéric Massart added a comment -

              Thanks Jason and Nadav for working on this, here are a few comments.

              1/ Jason, there must be something wrong in the patch (or I didn't look properly), because 2 of the commits (yours and Nadav's) are containing the same thing (or almost). Does your first commit need to be there?

              2/ There are few margin/alignment issues:

              The margin with the picture on smaller screen (ltr + rtl):

              Some labels go on the next line, some don't. Also the label on the next line should be right aligned (rtl):

              The description does not have any padding/margin with the picture (rtl + ltr):

              3/ I don't think responsive.less should have rules specific to one page. What I would suggest is to adapt the rule .dl-horizontal to RTL. For instance create a mixin .dl-horizontal-rtl() which contains the overrides for RTL, and use it as: .dir-rtl .userprofile dl.list

              { .dl-horizontal-rtl; }

              And not to lose the real bootstrap way of doing things: .dir-rtl .dl-horizontal { .dl-horizontal-rtl; }

              Check the differences with https://github.com/AbdullahDiaa/Bootstrap-RTL/blob/master/2.3.2/less%20RTL/type.less#L154 to create dl-horizontal-rtl.

              Cheers,
              Fred

              Show
              fred Frédéric Massart added a comment - Thanks Jason and Nadav for working on this, here are a few comments. 1/ Jason, there must be something wrong in the patch (or I didn't look properly), because 2 of the commits (yours and Nadav's) are containing the same thing (or almost). Does your first commit need to be there? 2/ There are few margin/alignment issues: The margin with the picture on smaller screen (ltr + rtl): Some labels go on the next line, some don't. Also the label on the next line should be right aligned (rtl): The description does not have any padding/margin with the picture (rtl + ltr): 3/ I don't think responsive.less should have rules specific to one page. What I would suggest is to adapt the rule .dl-horizontal to RTL. For instance create a mixin .dl-horizontal-rtl() which contains the overrides for RTL, and use it as: .dir-rtl .userprofile dl.list { .dl-horizontal-rtl; } And not to lose the real bootstrap way of doing things: .dir-rtl .dl-horizontal { .dl-horizontal-rtl; } Check the differences with https://github.com/AbdullahDiaa/Bootstrap-RTL/blob/master/2.3.2/less%20RTL/type.less#L154 to create dl-horizontal-rtl. Cheers, Fred
              Hide
              phalacee Jason Fowler added a comment -

              Thanks for that Fred, I have no idea why there are two commits from me, I only made one, as far as I know, unless something went pear shaped while I was porting from 2.6 to master... I will look over the code again and see what happened, and I will look at the other suggestions you made.

              Show
              phalacee Jason Fowler added a comment - Thanks for that Fred, I have no idea why there are two commits from me, I only made one, as far as I know, unless something went pear shaped while I was porting from 2.6 to master... I will look over the code again and see what happened, and I will look at the other suggestions you made.
              Hide
              phalacee Jason Fowler added a comment -

              as it seems my patches are making to problem worse, not better, I think I will pull them, and raise another issue to address them later. In the mean time I will push ahead with Nadav's original patch.

              Show
              phalacee Jason Fowler added a comment - as it seems my patches are making to problem worse, not better, I think I will pull them, and raise another issue to address them later. In the mean time I will push ahead with Nadav's original patch.
              Hide
              nadavkav Nadav Kavalerchik added a comment -

              Hi Guys

              I found some free time to go through some of my old RTL issues.
              Do you need any help?

              Show
              nadavkav Nadav Kavalerchik added a comment - Hi Guys I found some free time to go through some of my old RTL issues. Do you need any help?
              Hide
              phalacee Jason Fowler added a comment -

              I've just started fixing the issues Fred raised Nadav, I'll put this up for peer review later, and if you want, you can have a look over it and tell me what you think.

              Fred, I think the whole mixin in thing is a bit much, for something that is so simple any way.

              Show
              phalacee Jason Fowler added a comment - I've just started fixing the issues Fred raised Nadav, I'll put this up for peer review later, and if you want, you can have a look over it and tell me what you think. Fred, I think the whole mixin in thing is a bit much, for something that is so simple any way.
              Hide
              nadavkav Nadav Kavalerchik added a comment -

              Great. (Thank you Jason for taking over this issue!)

              Show
              nadavkav Nadav Kavalerchik added a comment - Great. (Thank you Jason for taking over this issue!)
              Hide
              nadavkav Nadav Kavalerchik added a comment -

              Jason, I have fetched your MDL-40786-master branch locally and it looks great.
              (See last attached screen capture)

              Show
              nadavkav Nadav Kavalerchik added a comment - Jason, I have fetched your MDL-40786 -master branch locally and it looks great. (See last attached screen capture)
              Hide
              nadavkav Nadav Kavalerchik added a comment - - edited

              Starting Peerreview. Looks great. (on master)

              Show
              nadavkav Nadav Kavalerchik added a comment - - edited Starting Peerreview. Looks great. (on master)
              Hide
              nadavkav Nadav Kavalerchik added a comment - - edited

              Finished (looks great). Please move it forward to integration.

              Show
              nadavkav Nadav Kavalerchik added a comment - - edited Finished (looks great). Please move it forward to integration.
              Hide
              poltawski Dan Poltawski added a comment -

              Hi Guys,

              1. I'm reopening this because I think that we should avoid a commit which introduces conflict markers in the history if we can. To be honest, since they are only tiny fixes to the original, I think you should squash them together.

              2. Since this is basically Nadavs work, it seems to defeat the purpose of the peer review to get him to review it! Perhaps we could let Fred have another go at this one?

              3. I'm no expert, but I would like to see some discussion of using a raw value (100px along with @horizontalComponentOffset1200). It seems like combining these could be fragile?

              cheers!
              Dan

              Show
              poltawski Dan Poltawski added a comment - Hi Guys, 1. I'm reopening this because I think that we should avoid a commit which introduces conflict markers in the history if we can. To be honest, since they are only tiny fixes to the original, I think you should squash them together. 2. Since this is basically Nadavs work, it seems to defeat the purpose of the peer review to get him to review it! Perhaps we could let Fred have another go at this one? 3. I'm no expert, but I would like to see some discussion of using a raw value (100px along with @horizontalComponentOffset1200). It seems like combining these could be fragile? cheers! Dan
              Hide
              phalacee Jason Fowler added a comment -

              The raw value thing is done through out the bootstrapbase less, and in the end, its one value, represented by a variable (@horizontalComponentOffset1200) and another value written in by default, 100px. WIll squish the code and put it back up for review by Fred or anyone else who wants it.

              Show
              phalacee Jason Fowler added a comment - The raw value thing is done through out the bootstrapbase less, and in the end, its one value, represented by a variable (@horizontalComponentOffset1200) and another value written in by default, 100px. WIll squish the code and put it back up for review by Fred or anyone else who wants it.
              Hide
              andyjdavis Andrew Davis added a comment -

              Have the testing instructions link to the specific screenshot that the tester should compare against. It is currently not very clear.

              I'd also prefer to see the tester test the same area in LTR as well, just to be absolutely sure that it is not being affected.

              Looking in theme/bootstrapbase/less/moodle/responsive.less the changes around line 133 are within a block that starts with this.

              // login page
              @media (min-width: 1200px) {

              The first few items appear to be login page specific but then there is more general stuff. Is this CSS in the wrong spot or is the comment wrong?

              Show
              andyjdavis Andrew Davis added a comment - Have the testing instructions link to the specific screenshot that the tester should compare against. It is currently not very clear. I'd also prefer to see the tester test the same area in LTR as well, just to be absolutely sure that it is not being affected. Looking in theme/bootstrapbase/less/moodle/responsive.less the changes around line 133 are within a block that starts with this. // login page @media (min-width: 1200px) { The first few items appear to be login page specific but then there is more general stuff. Is this CSS in the wrong spot or is the comment wrong?
              Hide
              phalacee Jason Fowler added a comment -

              I think the comment is wrong ...

              Show
              phalacee Jason Fowler added a comment - I think the comment is wrong ...
              Hide
              phalacee Jason Fowler added a comment -

              I will fix the instructions now.

              Show
              phalacee Jason Fowler added a comment - I will fix the instructions now.
              Hide
              phalacee Jason Fowler added a comment -

              Testing instructions fixed. Not sure if I should fix that comment or not. It seems trivial, so I would like to fix it, but not sure it's permitted ...

              Show
              phalacee Jason Fowler added a comment - Testing instructions fixed. Not sure if I should fix that comment or not. It seems trivial, so I would like to fix it, but not sure it's permitted ...
              Hide
              phalacee Jason Fowler added a comment -

              I think, with that login page thing, someone started off with it as the login page area, but others have been lazy and have added to it. I will leave it as is for now though. Submitting for integration.

              Show
              phalacee Jason Fowler added a comment - I think, with that login page thing, someone started off with it as the login page area, but others have been lazy and have added to it. I will leave it as is for now though. Submitting for integration.
              Hide
              damyon Damyon Wiese added a comment -

              Thanks for the patch - but I'm reopening this again for 2 reasons.

              1 (Minor) Arithmetic in less should be contained in parenthesis (Quote from from page of lesscss.org "Operations should only be performed within parentheses in order to ensure compatibility with CSS").

              2 (More important) The second is that these styles do not collapse properly on small screens. For LTR, on a small screen, the fields collapse to have the labels above the fields (all left aligned). For RTL with this patch, they just become right aligned, with the labels on the same lines as the fields which looks messy (they should be all right aligned with the labels above the fields).

              Show
              damyon Damyon Wiese added a comment - Thanks for the patch - but I'm reopening this again for 2 reasons. 1 (Minor) Arithmetic in less should be contained in parenthesis (Quote from from page of lesscss.org "Operations should only be performed within parentheses in order to ensure compatibility with CSS"). 2 (More important) The second is that these styles do not collapse properly on small screens. For LTR, on a small screen, the fields collapse to have the labels above the fields (all left aligned). For RTL with this patch, they just become right aligned, with the labels on the same lines as the fields which looks messy (they should be all right aligned with the labels above the fields).
              Hide
              cibot CiBoT added a comment -

              Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

              Show
              cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
              Hide
              phalacee Jason Fowler added a comment -

              I've made the changes as requested, resubmitting for integration now.

              Show
              phalacee Jason Fowler added a comment - I've made the changes as requested, resubmitting for integration now.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

              TIA and ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
              Hide
              phalacee Jason Fowler added a comment -

              Rebased

              Show
              phalacee Jason Fowler added a comment - Rebased
              Hide
              cibot CiBoT added a comment -

              Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              Hide
              samhemelryk Sam Hemelryk added a comment -

              This week it has landed. Many thanks guys

              Show
              samhemelryk Sam Hemelryk added a comment - This week it has landed. Many thanks guys
              Hide
              andyjdavis Andrew Davis added a comment -

              Works as described. Passing.

              Show
              andyjdavis Andrew Davis added a comment - Works as described. Passing.
              Hide
              marina Marina Glancy added a comment -

              Thanks for your hard work. Your code has now become a part of Moodle!

              Show
              marina Marina Glancy added a comment - Thanks for your hard work. Your code has now become a part of Moodle!

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    8/Jul/13