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

          Issue Links

            Activity

            Hide
            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
            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
            Nadav Kavalerchik added a comment -

            Rebased (24/7/2013)

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

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

            Reopening... thanks for your effort!

            Show
            Eloy Lafuente (stronk7) added a comment - LOL, same than https://tracker.moodle.org/browse/MDL-39682#comment-234848 Reopening... thanks for your effort!
            Hide
            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
            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
            Mary Evans added a comment -

            Looks OK so pushing to integration review

            Show
            Mary Evans added a comment - Looks OK so pushing to integration review
            Hide
            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 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 added a comment -

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

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

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

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

            Taking over from Nadav to get this through.

            Show
            Jason Fowler added a comment - Taking over from Nadav to get this through.
            Hide
            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
            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
            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
            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
            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
            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
            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
            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
            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
            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
            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
            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
            Nadav Kavalerchik added a comment -

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

            Show
            Nadav Kavalerchik added a comment - Great. (Thank you Jason for taking over this issue!)
            Hide
            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
            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
            Nadav Kavalerchik added a comment - - edited

            Starting Peerreview. Looks great. (on master)

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

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

            Show
            Nadav Kavalerchik added a comment - - edited Finished (looks great). Please move it forward to integration.
            Hide
            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
            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
            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
            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
            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
            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
            Jason Fowler added a comment -

            I think the comment is wrong ...

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

            I will fix the instructions now.

            Show
            Jason Fowler added a comment - I will fix the instructions now.
            Hide
            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
            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
            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
            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 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 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 added a comment -

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

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

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

            Show
            Jason Fowler added a comment - I've made the changes as requested, resubmitting for integration now.
            Hide
            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
            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
            Jason Fowler added a comment -

            Rebased

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

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

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

            This week it has landed. Many thanks guys

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

            Works as described. Passing.

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

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

            Show
            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:

                  Agile