Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UP-4622: Add offcanvas stickynav flyout #598

Merged
merged 2 commits into from Apr 21, 2016
Merged

UP-4622: Add offcanvas stickynav flyout #598

merged 2 commits into from Apr 21, 2016

Conversation

cousquer
Copy link
Contributor

@cousquer cousquer commented Nov 16, 2015

Add sticky nav like mySail but with notification and greeting on mobile
Restore optional Fly-out menu bootstrapized. (the menu toggle is on the left of each tab because it won't interfere with delete tab link)
Add a new optional off-canvas menu for the main nav.
in navigation.xsl you just need to set true for these param

Old Version

old

### New Version

new

@blamonet
Copy link

👍

@drewwills
Copy link
Contributor

Great to have this! I hope to have a chance to look shortly.

<xsl:otherwise>
<xsl:attribute name="class">container-fluid</xsl:attribute>
</xsl:otherwise>
</xsl:choose>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI One common approach that is done in other areas is to have something like
https://github.com/Jasig/uPortal/blob/uportal-4.2.1/uportal-war/src/main/resources/layout/theme/respondr/content.xsl#L94-L99
which is used later like at https://github.com/Jasig/uPortal/blob/uportal-4.2.1/uportal-war/src/main/resources/layout/theme/respondr/content.xsl#L119. This insures there is one place for specifying the CSS classes and HTML markup to reduce the chance someone later on will change one HTML element or attribute but forget to change the other. It's an alternate to what you have above but I think it reads a little easier IMHO.

Oh, and I see you do that later on.

@jameswennmacher
Copy link

+1 Overall looks great! I'm looking forward to seeing this merged in.

@cousquer
Copy link
Contributor Author

Hi James, hi All

In fact my approach was at two level with OffCanvas :

First, make it optional, I don't want to disturb people who like the vertical dropdown navigation on mobile. So, that's why all the offcanvas css classes are separated from the other css classes

Also the second level is to manage the future possibility to have perhaps on mobile rightOffcanvas, topOffcanvas, leftOffcanvas or downOffcanvas
(like the double or triple offcanvas of http://forecast.io on iphone, if you don't know this html5app, have a look, it's so great, no need to go in iTunesStore).
and I'll try to explain my code
if the offcanvas is activated, when someone clicks on the toggle menu two things happen first the visibility of the nav bar is activated and then in an other function, the offcanvas panel is open simply by toggle of a css class.
this means that you can imagine to open the panel for an other purpose if you want it for options in a portlet, aside blocks in portlet, etc.
you just need to position these blocks in the offcanvas on mobile and switch the visibility and open the panel. not sure if I am clear, sorry for my english, I'm french...

James you can improve the code as you want.

I will improve maybe the code in a couple of weeks with swipe right and swipe left functions on mobile but I need to test this, if it won't enter in conflict with your pull resquest with drag 'n drop portlet you've made which is very cool.

I'm doing baby steps

I have a doubt and a question for Drew about region.header-top what's in it ? and for which purpose, because the sticky nav will be perhaps positionned on it on mobile...

Sorry for "#portalCASLogin" it was the end of friday night...

@jameswennmacher
Copy link

@cousquer sounds great. Baby steps are good (and encouraged 😄 ).

RE the regions: see https://wiki.jasig.org/display/UPC/Respondr+Regions+Feature. Currently there are no portlets in the quickstart data set that use header-top. However it is available for use as a general region. I know we are using it for a multi-tenant client where the top has a logo branding that applies to all college campuses, and header left and right are used for a college-specific branding area and search.

@cousquer
Copy link
Contributor Author

@drewwills
Hi Drew,
Did you have time to look at this PR ?

@auxepaul
Copy link
Contributor

auxepaul commented Jan 7, 2016

+1

@jgribonvald
Copy link
Contributor

+1

And more with our new version of the portal we will use this PR

@cervomix
Copy link

cervomix commented Feb 9, 2016

This is definitely a must have

@cousquer cousquer changed the title Nojira add offcanvas stickynav flyout UP-4622: Add offcanvas stickynav flyout Mar 3, 2016
@cervomix
Copy link

Hi guys, why this hasn't been merged yet ? Any compatibility issues ? Cheers

@cousquer
Copy link
Contributor Author

Hi @cervomix
I do not know why.
For my part, everything works fine for my deployment and I don't think there is any compatibility issues. But I think they need to solve the most important problems first.

<channel fname="logout-launcher" unremovable="false" hidden="false" immutable="false" ID="n130"/>
<channel fname="portal-greeting" unremovable="false" hidden="false" immutable="false" ID="n120"/>
<channel fname="notification-icon" unremovable="false" hidden="false" immutable="false" ID="n110"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why reorder portal-greeting & notification-icon? (Can probably answer for myself once I review the changes... just making a note.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was to preserve the visual order of the items because Ihave change the css alignment rules

@drewwills
Copy link
Contributor

I'd say this implementation of the small-display menu is vastly preferable to the old version. I'm not sure we need the old version at all... I think I'm inclined to cut it, in the interests of simplicity.

@jgribonvald
Copy link
Contributor

Could it be integrated in the upcomming release ?
👍

@drewwills
Copy link
Contributor

@jgribonvald -- That is my hope & intention.

@drewwills drewwills merged commit da8f3a7 into uPortal-Project:master Apr 21, 2016
@jgribonvald
Copy link
Contributor

thanks a lot Drew for that !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants