michael-milette / moodle-theme_gcweb

This is the GCWeb 4.0.30 theme for Moodle LMS and IOMAD 3.9, 3.10 and 3.11 used by the Government of Canada. Please fork and star. For premium support and 4.x compatibiity, contact us at https://www.tngconsulting.ca/contact
https://www.tngconsulting.ca/home/open-source-gcweb-theme-for-moodle-lms-for-the-government-of-canada/
Other
3 stars 6 forks source link

Nav-Drawer Button Position #19

Open sameer-ah opened 3 years ago

sameer-ah commented 3 years ago

Hi @michael-milette,

I have a suggestion regarding the position of hamburger icon (used to show/hide left menu)

image

It would be better to move this icon near to left menu as in the boost theme where icon is right on top of menu.

image

The current position of the icon in GCWeb theme is a bit confusing, it is at the opposite end of actual menu. At first glance, it is difficult to understand the purpose of this icon.

Please share your thoughts on this suggestion.

Thank you, Sameer

michael-milette commented 3 years ago

Hi @sameer-ah ,

I agree that it is not in an ideal spot however placing it at the top left corner would not fit with the GCWeb for Applications common look and feel.

Let me know if you have any other suggestions.

Best regards,

Michael

sameer-ah commented 3 years ago

@michael-milette, you are right, I am also struggling to find a better alternate position.

What about before application name, close to left menu?

image

Although not perfect but this is best alternate I have come up with.

Thanks, Sameer

michael-milette commented 3 years ago

Hi @sameer-ah ,

I thought of that too. In fact, I had it there early on in the theme's development. However it failed accessibility testing since it appeared before the for first heading and it got covered when open on mobile devices making it impossible to close. If you can find a way to add a button to the top of the nav drawer (that is what that side menu is called in Moodle), it might be possible to work out accessibility issues.

Best regards,

Michael

sameer-ah commented 3 years ago

Hi @michael-milette,

We could have something like

image

or

image

The real issue is hide mode, where it is

image

Icon not looking good at top of Government of Canada logo but could be an option.

What do you think?

Thanks, Sameer

michael-milette commented 3 years ago

I agree. It does not look good and would likely fail CLF compliance and WCAG testing.

Let's take this one step at a time. See if you can implement a close [X] type button in the drawer. If you can, I think that would be a very good improvement on its own as it would improve both usability and accessibility.

sameer-ah commented 3 years ago

Hi @michael-milette,

It is a bit tricky to add a close [X] button to nav drawer.

Nav drawer is rendered by using boost theme template. https://github.com/michael-milette/moodle-theme_gcweb/blob/2f30c462abef0ed1f847e3fd35f0d419d5c20d34/templates/footer.mustache#L89

Adding a hamburger icon was easier, as it was to always stay at the top left corner of page and must always be visible whether nav drawer is shown/hidden.

In case of close button, it does not look good at the top left

image Instead, it is much better to have it like

image

But now the issue is how to hide this button when nav drawer is hidden because it is not actually part of nav drawer. I have only managed to put it on top of nav drawer.

Please give some suggestions in case I missed something or there is a better way of doing it.

Thanks, Sameer

michael-milette commented 3 years ago

Do you have a working close button? If so, you can put it in the drawer. The code for the drawer is in one of the mustache template files.

Michael

sameer-ah commented 3 years ago

Based on my understanding of the code, drawer is rendered from footer.mustache template

https://github.com/michael-milette/moodle-theme_gcweb/blob/2f30c462abef0ed1f847e3fd35f0d419d5c20d34/templates/footer.mustache#L89

In the previous screenshots, I have put the close button code there.

Yes, there is nav-drawer.mustache template in GCWeb theme but whatever code I put in there is not getting rendered. On the other hand, if I put the close button code inside nav-drawer.mustache template for boost theme then it gets rendered and it also gets hidden when drawer is closed.

Moreover, if I change code at line https://github.com/michael-milette/moodle-theme_gcweb/blob/2f30c462abef0ed1f847e3fd35f0d419d5c20d34/templates/footer.mustache#L89 to {{> theme_gcweb/nav-drawer }} then it works as expected and I can put close button code inside GCWeb theme nav-drawer.mustche template.

Please correct if I am not getting it right.

Thanks, Sameer

michael-milette commented 3 years ago

Hi @sameer-ah ,

Did you get a Close button to work? The location and look of button is easy to address. However, if you can't make the button work, there isn't much point in discussing where it will be and what it will look like.

Best regards,

Michael

sameer-ah commented 3 years ago

Hi @michael-milette,

Yes, I have close button working. The only issue is while switching between hamburger and close button, it takes an extra click to open/close drawer. I am not able to figure out what is causing this issue.

Thanks, Sameer

michael-milette commented 3 years ago

Hi @sameer-ah

I suspect that you are closing the drawer yourself instead of replicating the way Moodle does it.

The problem with this approach is that, internally, it leaves Moodle drawer status in a different state than it actually is physically.

The result is that your first click then changes the status without any visible change and then the second click action is back in sync with both the drawer and the button's state.

Best regards,

Michael

sameer-ah commented 3 years ago

Hi @michael-milette,

Thank you for sharing the details.

I copied the same HTML code from template for existing hamburger button, the only difference is the icon used and some CSS styles. I was expecting it to behave similarly.

I also did a small test where I duplicated the existing hamburger button at the same place and got the same extra one click behavior as with close button.

Thanks, Sameer

michael-milette commented 3 years ago

Hi @sameer-ah ,

I don't have time to work on this right now as I am involved in other Moodle-related projects. However, please do let me know when you figure it out and so that we can move forward. If you need premium support from me, please let Michael and Daniel know.

Best regards,

Michael

sameer-ah commented 3 years ago

Hi @michael-milette,

I really appreciate your time.

It is not something urgent that needs to be fixed/resolved. I had sometime so I thought of doing this to contribute to your plugin repository. I will continue looking into extra click issue and try to resolve at my own.

Thank you, Sameer

michael-milette commented 3 years ago

Sounds good. I can tell you that I have been through everything you've done so far. If you can figure out how to make a workable close button, I would definitely be interested in working on this with you to move the nav-drawer button to the left side where I think it would look better.

I also tried creating a button which would click the nav-drawer button as suggested by some other Moodle developers, but it didn't work. I felt like I was so close to getting it working but I ran out of time. That was my latest attempt over this past summer.

For simplicity and to ensure that it is not being blocked by WET-BOEW, I would suggest developing it with the default Moodle Boost theme. If you can get it working there and not in GCWeb, at least you will have narrowed down the cause.

Have you considered that this might be AMD related? Read-up on developing JavaScript in Moodle at: https://docs.moodle.org/dev/Javascript_Modules

Best regards,

Michael

sameer-ah commented 3 years ago

Thank you for suggesting to add close button to Boost theme, I was also thinking the same.

I will also read further about AMD. The closest I got was to find the javascript code (drawer.js file) which open/closes drawer but I was unable to put breakpoint in the code to figure what could be wrong. Above link will definitely help me learn more about javascript in Moodle.

Thanks, Sameer

michael-milette commented 3 years ago

Hi @sameer-ah

For future reference, here is a mock-up of my vision of what the top of the nav-drawer could look like:

image

I used a chevron instead of a X Close button which seems more intuitive to me since it would be consistent with the animation. I also added a title for the nav drawer to visually balance it out and aid with accessibility.

On a desktop, the user should be able to close the drawer using either the nav-bar icon or the chevron. However, in mobile view, only the chevron could be used.

Optional enhancement for mobile view: It might be nice if you could also close the drawer by tapping anywhere outside the drawer. I believe that this would help address a usability issue where the user open the nav drawer, scroll down to the bottom but doesn't find what they are are looking for. To close the menu, they currently need to scroll to the top of the page. With this enhancement (again, only available in mobile view), they you would simply tap outside the nav drawer to close it.

Best regards,

Michael

sameer-ah commented 3 years ago

Hi @michael-milette,

Thank you for sharing all the details.

Let me first sort out the extra click issue. In the next step, I will replace close button with chevron and move current button to nav-bar. In the last step, we will target the mobile view and looking further into enhancement suggested by you.

Thank you, Sameer

sameer-ah commented 3 years ago

Hi @michael-milette,

Thank you for pointing me in the right direction, it was indeed an AMD issue.

The problem was in Moodle boost theme code. I have opened an issue at Moodle Tracker. Kindly have a look at the issue and add your comments in case it needs further clarification.

Coming back to our changes, obviously the above issue needs to be fixed first. But since it is Moodle core change, I would suggest to proceed ahead while waiting for the fix. I will make changes and create a pull request. It will be up to you to decide when is good time to incorporate the changes in GCWeb theme. What do you say?

Thank you, Sameer

michael-milette commented 3 years ago

Hi @sameer-ah

Sounds good. I suggest creating a new feature branch of GCWeb until it is integrated.

Here are a few suggestions for you tracker ticket:

  1. Moodle 3.9 is in maintenance mode which means it will only receive security fixes. Your issue will not get fixed in 3.9. I suggest you mark it as a Moodle 3.11 issue as it is still receiving bug fixes. Of course you should make sure that it is still an issue in 3.11. The GCWeb theme should be compatible with 3.11 according to my testing to date.

  2. Submit a patch yourself for all supported versions of Moodle including the forthcoming v4.0. You can create one for 3.9 too. Moodle is very much community supported so issues that include fixes are more likely to be integrated once it gets past their QA testing reviews.

  3. Include testing instructions. They won't integrate unless you can show QA testers how to verify that it not only works but also doesn't break anything in the process. There is a place for these instructions when edititing the ticket.

Let me know if you have any questions.

Best regards,

Michael

sameer-ah commented 3 years ago

Hi @michael-milette,

Thank you for your suggestions to improve tracker ticket. I have made all of the suggested changes.

I have implemented nav-drawer changes as per your mock screen. Can I please create a pull request for your review? You said changes will be kept in a feature branch. Should I share my feature branch or you will create a new feature branch in your repository and I will create a pull request to that feature branch.

Thank you, Sameer

michael-milette commented 3 years ago

Hi @sameer-ah

I recently came across a premium theme called Impression Boost that took a bit of a different approach. They lowered the nav drawer below the menu so that only the content is pushed over, not site header. See: image

I also came across a screenshot of the free Moove theme where they seem to have implemented a "Minimize menu" button at the top of the Nav Drawer. I don't think this is the default layout for the theme, It might be worth investigating to see how they did it without having to modify Moodle. For all I know, this might only be available in their premium Moove Pro version. image

Just a couple of potential things for you to consider and explore.

Best regards,

Michael

michael-milette commented 3 years ago

Hi @sameer-ah

You cannot submit a patch file for integration into Moodle. They don't work that way. You need to create a feature branches in your fork of Moodle on GitHub and then submit the links in your ticket at https://tracker.moodle.org/browse/MDL-72645 . Please read the developer documentation on Process at https://docs.moodle.org/dev/Process . You will also need to maintain your patches by rebasing and addressing any concerns the testers and integrators have until it is accepted and integrated.

I would also suggest that you make your argument for integration more compelling. They are not going to do any development such as duplicating the button. You need to submit everything that they will need to them. They look at many patches every day and will not give your request more attention than it deserves, especially since everything works as is right now. Show them a reason why it is a must, not just a nice to have. For example, a compelling reason might be to address a bug or correct an accessibility issue.

Best regards,

Michael

sameer-ah commented 3 years ago

Hi @michael-milette,

Thank you for suggestions to explore Impression Boost and Moove theme. Actually, at start I was considering looking into other themes where someone could have implemented a minimize/hide button. But there are so many themes in moodle and checking each one to see if anyone has implemented something very specific, seemed to much hassle to me. I really appreciate sharing the two themes known to you with similar functionality. I will look into the code to figure out how the implementation is done and try to do the same for GCWeb theme.

Thank you for more suggestion on improving Moodle tracker ticket. Based on my understanding, as member of Users group, I can only create a patch. The way I created a patch was based on Moodle How to create a patch documentation. I will definitely follow your guidelines and create a feature branch with the fix/patch. Moreover, I will also create a branch to replicate the issue as well. Along with that I will also add some more arguments to ticket description elaborating further on accessibility issue.

Let me first explore the above two themes before spending more time to update the ticket.

Thank you, Sameer

sameer-ah commented 3 years ago

Hi @michael-milette,

I have looked at the two themes. Both of these have only one nav-drawer button so it is working without modifying Moodle code.

Both of them are using fixed/sticky header approach. GCWeb theme can also use the same approach by using single button without waiting for Moodle code fix. It could be something like

nav-drawer open: image

nav-drawer close:

image

My first guess is it will bring many more CSS changes and will need thorough testing to make sure nothing else is broken especially mobile view. But definitely this is a potential better option to consider. Please share your ideas on it.

Thank you, Sameer

michael-milette commented 3 years ago

We you able to configure the Moove theme so that it appears like: image

I am sure that, when the menu is hidden, there isn't an icon that says "Minimize menu".

If you did get it to work that way, please let me know what the setting is to enable that layout and I will take a look.

Michael

sameer-ah commented 3 years ago

No, I did not find "Minimize menu" anywhere. Their premium site demo page (https://pluginstore.conecti.me/plugins/theme_moove) has live preview but the nav-drawer button icon and behavior is same.

michael-milette commented 3 years ago

H @sameer-ah ,

I have been doing a lot of work with the Trema theme recently to make it more WCAG 2.1 compliant when combined with a several Moodle patches that have yet to be accepted and integrated. It has convinced me that the best approach to this issue is to move the nav drawer down as mentioned https://github.com/michael-milette/moodle-theme_gcweb/issues/19#issuecomment-929650195

Perhaps it would be good to do for a Moodle 4.0 version of the theme. I've already started checking GCWeb for compatibility issues with Moodle 4.0 Alpha. So far (I just started), it seems to work but lacks many of the new features included in 4.0. Obviously it will need more work.

Michael

michael-milette commented 3 years ago

Hi @sameer-ah

Found one! Here is a theme that has a the nav drawer toggle on the page and a close button in the nav drawer: https://mb2themes.com/themes/macro-learning/

Unfortunately you cannot use this theme with the drawer open. Hopefully that isn't a limitation. See what you can do with it.

MIchael

michael-milette commented 3 years ago

Hey @sameer-ah

Here is another idea. Instead of having two button, why not reposition the nav drawer button when the drawer is open and then put it back in its normal place when the nav drawer is closed? Then you don't need to figure out how to add a second button.

Michael

sameer-ah commented 2 years ago

Hi @michael-milette,

Thank you for more suggestions. If you decide the best approach is what was previously discussed then I did make an initial set of changes to GCWeb theme. Please do let me know in case you do not have access to those repositories (option 1, option 2).

I will also explore mb2themes and try to make something similar in GCWeb theme.

Thanks, Sameer

michael-milette commented 2 years ago

Hi @sameer-ah ,

Your Option 1 is similar to mb2theme except that a) I like your implementation better, however b) its buttons work better - they don't need to be clicked twice to activate.

That said, I think that I prefer for option 2 (feature-19-a). From an accessibility point of view, I think the positioning is better and, should someone open or close the side drawer, no additional navigation by keyboard or mouse is required in order to toggle it again. Also, the toggle works properly in your prototype. image

Unfortunately, it breaks much of the header when viewed on a tablet and smartphone. image

At first glance:

Also, do you think the Nav Drawer should be below the custom menu? If people work on tablets, the menu could become pretty crowded. Thoughts?

I think this is a good step in the right direction. I realize it is a work in progress. :-)

Best regards,

Michael

sameer-ah commented 2 years ago

Hi @michael-milette,

Thank you for reviewing both options. Until Moodle code is fixed, we cannot proceed with option 1.

For the option 2, yes more work is needed to make it responsive. When I will get sometime, I will definitely work on it further.

For the Nav Drawer position, I first thought you meant Nav Drawer button. If you are actually referring to Nav Drawer position, I am not able to picture how it would look like if we move Nav Drawer below custom menu. I also can't think of any advantage of this change in position. May be I did not completely understood what you want to do.

Thanks, Sameer