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

feat(organizations): add theming/story for Mantine Select TASK-1379 #5414

Merged
merged 15 commits into from
Jan 21, 2025

Conversation

pauloamorimbr
Copy link
Contributor

@pauloamorimbr pauloamorimbr commented Jan 9, 2025

🗒️ Checklist

  1. run linter locally
  2. update all related docs (API, README, inline, etc.), if any
  3. draft PR with a title <type>(<scope>)<!>: <title> TASK-1234
  4. tag PR: at least frontend or backend unless it's global
  5. fill in the template below and delete template comments
  6. review thyself: read the diff and repro the preview as written
  7. open PR & confirm that CI passes
  8. request reviewers, if needed
  9. delete this section before merging

📣 Summary

Theming for Mantine's Select was added to match our KoboSelect visual

📖 Description

  • Story was added in storybook under Mantine/Select
  • Theming was added for Mantine's Select component
  • A new Select component was created to wrap Mantine's Select and add visual functionalities:
    • Select chevron changes when select list is opened
    • Clear button appears by the side of the chevron

⚠️ There are some behaviors and styling that differs from our current select implementation. I checked with Tessa regarding the styling and some of the behaviors, but we still have 2 BUG that I tried hard but wasn't able to fix by using the MantineSelect as it is, which may cause us to end up adding our own implementation based on Matine's one:

  • When you have the dropdown opened and press TAB, it won't close, and you cannot do it with the ESC key anymore.
  • When clearable is enabled and an item is selected, the chevron becomes unclickable

Also:

  • Members table role selector was updated to use the Select component as a POC
  • Some logic was changed to be simplified and match the new implementation
  • ⚠️ A LoadingOverlay is being used in the members table to keep the user from doing other actions while processing the role change. The functionality seems good, but we need to setup the Loader theme as well for it to be used like that. Preferably in another task.

👀 Preview steps

  • Use storybook to check the visual and basic functionality
  • Check Organization's Members table. It's recommended to change other user's role for testing.

@pauloamorimbr pauloamorimbr self-assigned this Jan 9, 2025
@pauloamorimbr pauloamorimbr marked this pull request as ready for review January 9, 2025 20:31
Copy link
Contributor

@Akuukis Akuukis left a comment

Choose a reason for hiding this comment

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

LGTM 👌 not tested

jsapp/js/theme/kobo/Select.module.css Outdated Show resolved Hide resolved
jsapp/js/theme/kobo/Select.module.css Outdated Show resolved Hide resolved
jsapp/js/theme/kobo/Select.module.css Outdated Show resolved Hide resolved
jsapp/js/components/common/Select.stories.tsx Outdated Show resolved Hide resolved
@jamesrkiger
Copy link
Contributor

Weird issue here:

image

To reproduce:

  1. Open up the select component for an org member (member role specifically)
  2. Use arrow down to navigate to member option
  3. Move the mouse to highlight the admin option
  4. Notice that the text color on 'member' remains white, so it basically disappears.

Obviously this is an edge case and I'm not sure if it's really a bug or if it's more a question of how our hover and focus style choices are interacting. Let me know what you think.

@jamesrkiger
Copy link
Contributor

Can we make it so the select closes when a user tabs to the next element? On the members table, if I open the select then tab to the member actions button, the select stays open and now I can't close it with 'esc'. I am noticing that this is also how the select behaves in Mantine examples, unfortunately, but I do think it's wrong and would be worth fixing if possible.

Base automatically changed from kalvis/mantine-setup to main January 15, 2025 08:50
@Akuukis Akuukis requested a review from p2edwards as a code owner January 15, 2025 08:50
@pauloamorimbr pauloamorimbr force-pushed the pamorim/task-1379-create-new-select-component branch from dc40a00 to 65f23c5 Compare January 16, 2025 21:01
@pauloamorimbr
Copy link
Contributor Author

Can we make it so the select closes when a user tabs to the next element? On the members table, if I open the select then tab to the member actions button, the select stays open and now I can't close it with 'esc'. I am noticing that this is also how the select behaves in Mantine examples, unfortunately, but I do think it's wrong and would be worth fixing if possible.

I tried hard but I just wasn't able to fix this behavior. I came to a point where it would be better to just get Mantine's select code and change it's implementation to add our needed behaviors and have a better control to fix this. 😞

We may end up doing just that in a future task.

@pauloamorimbr
Copy link
Contributor Author

Weird issue here:

image

To reproduce:

  1. Open up the select component for an org member (member role specifically)
  2. Use arrow down to navigate to member option
  3. Move the mouse to highlight the admin option
  4. Notice that the text color on 'member' remains white, so it basically disappears.

Obviously this is an edge case and I'm not sure if it's really a bug or if it's more a question of how our hover and focus style choices are interacting. Let me know what you think.

Just to mention that in geenral some styles were changed (this one you found was really a bug tho) so we aren't really following the current koboSelect styling in favor of following Tessa's indications.

Copy link
Contributor

@jamesrkiger jamesrkiger left a comment

Choose a reason for hiding this comment

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

Would appreciate it if could deprecate the old KoboSelect story. Other than that, lgtm

@pauloamorimbr pauloamorimbr force-pushed the pamorim/task-1379-create-new-select-component branch from 9246a21 to 13bd383 Compare January 21, 2025 20:32
@pauloamorimbr pauloamorimbr merged commit 910e66a into main Jan 21, 2025
7 checks passed
@pauloamorimbr pauloamorimbr deleted the pamorim/task-1379-create-new-select-component branch January 21, 2025 20:36
pauloamorimbr added a commit that referenced this pull request Jan 21, 2025
…Overlay TASK-1464 (#5425)

### 📣 Summary
Theming for Mantine's Loader and LoadingOverlay was added to match
Kobo's current LoadingSpinner for general purpose.

### 📖 Description
Both 'regular' and 'big' variant were added with its respective type
names to be used with Mantine's `Loader` and `LoadingOverlay`
components.
To simplify the implementation I decided to simply use the existing
`LoadingSpinner` component instead of extracting it's contents.
Since Mantine's `Loader` doesn't have the `message` parameter, right now
I'm fixing it on `false` so it won't appear. We may need to implement a
Wrapper in the future to add a message, depending on our future needs.

* `LoadingOverlay` is already in use at
#5414


### 👀 Preview steps
- Use storybook to check the visual of the component and compare with
the existing loader in use in the app.
@pauloamorimbr pauloamorimbr changed the title feat(organizations): add theming/story for Mantine Select TASK-1382 feat(organizations): add theming/story for Mantine Select TASK-1379 Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants