-
-
Notifications
You must be signed in to change notification settings - Fork 941
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
SAK-49631 - "Exit Student View" not working on other tabs except "Overview" tab #12364
Conversation
…rview" tab Some ideas for review
I probably should look at that |
I said the same thing to myself when I did this PR for the updated portal. I hope someone will tell me if there's a better way to do it :). I like it. This is better than what I did. Thanks @jonespm Do you mean, The Overview tool shouldn't have the option for role-swap? I think the negative margins were there to align the dropdown. I wasn't happy with it either. |
library/src/skins/default/src/sass/modules/tool/portal/_portal.scss
Outdated
Show resolved
Hide resolved
….scss Co-authored-by: Kunal Jaykam <50500283+kunaljaykam@users.noreply.github.com>
Thanks for the comments, I added your change and also changed it to check for isHomePage rather than the double column which seems more straightforward since this is the page with 99% of the issue. I think it's looking pretty nice now. I checked it also on mobile view and seems pretty nice there too. |
Thanks! with your fix. I think we can clear some CSS and JS as well. I have created a pull request on your branch for the same. Please take a look: jonespm#2 |
Removed Unnecessary codes, used bootstrap class instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks @jonespm
Some ideas for review.
I'm going to leave this as draft for now and am happy if someone does this differently or if this is almost good and I can fix it up.
Some things I didn't like:
I don't think these dropdowns should appear on the overview (iframe) divs but I don't see a better way to detect this other than
$pageColumnLayout == 'col1'
I'm also not sure why some of these negative margins were in there and they made the link no longer appear.
I'm not sure if there's any other edge cases here or if there's a better place this dropdown could be placed.
Before switching
In view student mode