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.

Sign up for GitHub

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

Jump to bottom

move user table params to the recoil state #519

Merged
merged 3 commits into from
Jul 26, 2022
Merged

move user table params to the recoil state #519

merged 3 commits into from
Jul 26, 2022

Conversation

nebs-dev
Copy link
Contributor

Resolves #494

@nebs-dev nebs-dev requested a review from kristoferlund July 20, 2022 13:37
@nebs-dev nebs-dev self-assigned this Jul 20, 2022
kristoferlund
kristoferlund requested changes Jul 25, 2022
View reviewed changes
Copy link
Member

@kristoferlund kristoferlund left a comment

Choose a reason for hiding this comment

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

Works great!

I had one comment regarding duplication of roleOptions.

And, add the x icon to the SearchInput component, inside the text input frame. similar to how the magnifying glass is done but opposite side.

image

packages/frontend/src/pages/Users/components/UsersTable.tsx Outdated
value: string;
label: string;
}

const roleOptions = [
Copy link
Member

Choose a reason for hiding this comment

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

You are maintaining two roleOptions lists. One here and one in model/users.ts. One should be enough. Consider removing this list and exporting the one found in model.

@nebs-dev nebs-dev requested a review from kristoferlund July 26, 2022 09:08
kristoferlund
kristoferlund approved these changes Jul 26, 2022
View reviewed changes
Copy link
Member

@kristoferlund kristoferlund 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!

@kristoferlund kristoferlund merged commit a5057ae into main Jul 26, 2022
@kristoferlund kristoferlund deleted the bug/user_page_back_state branch July 26, 2022 16:00
@kristoferlund kristoferlund mentioned this pull request Aug 24, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kristoferlund kristoferlund kristoferlund approved these changes

Assignees

@nebs-dev nebs-dev

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The User page should remember state when navigating to the User detail page and back
2 participants
@nebs-dev @kristoferlund