-
Notifications
You must be signed in to change notification settings - Fork 168
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
session expired modal #933
Conversation
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.
Alright, nice work so far. Revert the rest apart from sessionExpired
and it's jss. Also rename it to sessionExpiredModal
Simply create a new route called session-update
or anything better and link it to this component.
Then inside a useEffect
in either App.js
or PageWrapper.jsx
you redirect to this route once the token is expired.
Let's see how that goes
Am still looking for a replacement of this useEffect
. It's not as fast as we would want it to be
Hey @srish @DonPresh @coderatomy here is the look of the result of the implementation. Please I'll like a review on this. |
Hey @yokwejuste, please note my comment above and in the issue description. They contain a clear workaround this issue plus the reasons why. If you wouldn't like to follow that pattern, please provide a proper explanation for this approach wrapping all different scenarios and we see the way forward For the design, please also note the blurred background and it is actually better that way, thanks to @DonPresh's design integrity. That's why I insist on calling it |
Thanks for this @coderatomy, I'm jumping on to this today. :) |
overflow: 'hidden', | ||
}, | ||
cardHeader: { | ||
backgroundColor: '#f44336', |
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.
Use color from theme
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.
This color is left. This should be our primary color red
zubhub_frontend/zubhub/src/components/sessionExpired/sessionExpired.Style.js
Outdated
Show resolved
Hide resolved
zubhub_frontend/zubhub/src/components/sessionExpired/sessionExpired.Style.js
Outdated
Show resolved
Hide resolved
@yokwejuste I have left a few comments on the design. Also how does this look in responsive mode? I think we need some uniformity in the design across the site. Please try to use as many standard MUI components as possible which we can then theme. |
Got it, thanks, I'm on it right now |
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.
few comments mostly about proper translation. Nice work @yokwejuste !
zubhub_frontend/zubhub/src/components/sessionExpired/sessionExpired.jsx
Outdated
Show resolved
Hide resolved
zubhub_frontend/zubhub/src/components/sessionExpired/sessionExpired.jsx
Outdated
Show resolved
Hide resolved
zubhub_frontend/zubhub/src/components/sessionExpired/sessionExpired.jsx
Outdated
Show resolved
Hide resolved
zubhub_frontend/zubhub/src/components/sessionExpired/sessionExpired.jsx
Outdated
Show resolved
Hide resolved
FYI @tuxology @NdibeRaymond @yokwejuste. Since #931 we are recording the I haven't tested this PR locally yet, but I believe that will be the experience |
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.
Left one minor comment. Also please check comment from @coderatomy about the session-expired route issue #933 (comment) Can you test it and let me know if this is the case? If yes, lets fix it.
overflow: 'hidden', | ||
}, | ||
cardHeader: { | ||
backgroundColor: '#f44336', |
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.
This color is left. This should be our primary color red
@yokwejuste Also, we've left the svg. Lets remove this #933 (comment) |
@tuxology I think it looks good now |
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 now 🌈
Summary
Description of PR here...
Closes #906
Changes
Screenshots
Progress