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

Added reset password, which required a lot of changes and refactors #35

Merged

Conversation

SailingSteve
Copy link
Member

This was a very messy merge...
This is 95% done, but checking in so that we don't diverge so much again.
We diverged a bit with admin rights for sessions. I left all the new stuff in parallel with the old stuff (server and client) and I'll migrate over to the new way represented as captureAccessRightsData on the client side.
Still need to change over the MUI styles in the ResetYourPassword.jsx from MUI templates to our styles (they look different, and the difference does not add value).
Navigating to /Tasks currently leaves the HeaderBar menu confused, will fix this.
Spurious "Please sign in" after resetting password, will fix this.

This was a very messy merge...
This is 95% done, but checking in so that we don't diverge so much again.
We diverged a bit with admin rights for sessions.  I left all the new stuff in parallel with the old stuff (server and client) and I'll migrate over to the new way represented as captureAccessRightsData on the client side.
Still need to change over the MUI styles in the ResetYourPassword.jsx from MUI templates to our styles (they look different, and the difference does not add value).
Navigating to /Tasks currently leaves the HeaderBar menu confused, will fix this.
Spurious "Please sign in" after resetting password, will fix this.
More testing needed -- the steps are pretty complex, with the need for immediate `useRef()` variables in some places to avoid (always losing) race conditions.
Copy link
Member

@DaleMcGrew DaleMcGrew left a comment

Choose a reason for hiding this comment

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

Looks good @SailingSteve. 👍

${(largefont) ? 'font-size: 1.1em;' : 'font-size: .8em;'}
${(titlecell) ? '' : 'font-weight: 550;'}
border-bottom: ${(props) => (props?.$titleCell ? ';' : '1px solid #ccc;')}
font-size: ${(props) => (props?.$largefont ? '1.1em;' : '.8em;')};
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to only pass in the property names if they have a value, like this:
${(props) => (props?.$titleCell ? '' : 'border-bottom: 1px solid #ccc;')}
${(props) => (props?.$titleCell ? '' : 'font-weight: 550;')}

One can never be sure that Firefox or Opera will treat properties defined like this as "don't do anything", so I'd rather not send this css to the DOM:
border-bottom: ;
font-weight: ;

@DaleMcGrew DaleMcGrew merged commit 14fb862 into wevote:develop Feb 26, 2025
0 of 2 checks passed
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.

2 participants