search

uPortal-Attic / uportal-home

Alternative UI for Apereo uPortal (originally built for MyUW)
http://uportal-project.github.io/uportal-home/
Apache License 2.0
25 stars 27 forks source link

fix: use region-main in static and exclusive mode #760

Closed davidmsibley closed 6 years ago

davidmsibley commented 6 years ago

Addresses MUMUP-3231

before: image

after: image

Contributor License Agreement adherence:

davidmsibley commented 6 years ago

So, I don't have the full context, and haven't bothered to run through the git history to determine when and why this broke. But I identified the cause and fixed as least-invasive as I could.

The div holding the ng-controller directive doesn't have any display style specified, which causes it to default to block. This breaks it out of the flex layout, and sizes the div per the inner content.

Knowing that, there are a couple approaches to fixing this: restructure the html to match our intended structure, or style the element. I didn't feel like doing the research and trial and error to see if restructuring was possible, and on first glance it seemed appropriate that we wrap the frame page in the exclusive/static controller. So I look to styling the element to get it back into the flex layout. We need two things to re-flex it: display: flex; flex: inherit; (technically, we only need flex: inherit; as that will automatically set display to flex. But it's good to be explicit). At that point, I could either directly style the element (which is bad practice), or give the element a class and introduce a new css rule or apply an existing rule. I looked and right next to the frame-page css rule, there's another rule that effectively applies the flex layout: https://github.com/uPortal-Project/uportal-app-framework/blob/master/components/css/buckyless/frame.less#L58

I went with that over adding another css rule, just because I find css messy and I didn't want to add a one-off rule. Arguably, a one off rule is better than coupling a random div to a rule that wasn't made for it, but since this div plays the same role in the layout as the .region-main (which is an ancestor of this div), I think we can get away with it. I'd rather leave the overall css strategy up to @thevoiceofzeke. Since he's not here at the moment, I went for the least invasive change.