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

Add modals from Tobira #5

Merged
merged 19 commits into from
May 29, 2024
Merged

Add modals from Tobira #5

merged 19 commits into from
May 29, 2024

Conversation

Arnei
Copy link
Member

@Arnei Arnei commented May 23, 2024

Adds Modal and ConfirmationModal components and their types to appkit. Also adds a bunch of other stuff from Tobira that ConfirmationModal in particular requires.

Additionally:

  • Updates the opencast eslint dependency
  • Updates the emotion dependency
  • Introduces our color config to the Spinner component.

Points to consider:

  • For now I have only ripped these components out of Tobira, making sure they still work with Tobira. I have not yet attempted to actually use them in other tools. Doing so may result in further changes.
  • This adds new colors to our scheme, directly copied from Tobira. I have not yet considered if these colors are reasonable for other tools.
  • Some of these components had translations, which have been turned into props. Do we want a translation project for appkit? Is that feasible?

Arnei added 5 commits May 23, 2024 11:31
Adds the "Modal" component from Tobira.
The component is ported with the minimum
amount of required changes. Translations
string will have to be provided in their
translated form.
Adds another button component from
Tobira, with the goal of using it in the
confirmation modal from Tobira. The
button can be normal, happy or danger.
As how it is in Tobira.
Adds a nice little error box from Tobira to
render errors in. Also adds the card component
which is required for that. Used by confirmation
modal.
Adds the confirmation modal from tobira
Added missing colors, changed unused colors.
Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

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

Thanks for making these available in appkit!

I left some comments of things I think should be changed still. Regarding the translations: I think its fine just passing all strings to the component for now. Figuring out how to get i18next into appkit and make it work nicely when included in an app... is too much work now. The vast majority of duplicate code will already be eliminated by this.

package.json Show resolved Hide resolved
src/modal.tsx Outdated Show resolved Hide resolved
src/modal.tsx Outdated Show resolved Hide resolved
src/modal.tsx Outdated Show resolved Hide resolved
src/modal.tsx Show resolved Hide resolved
src/styledButton.tsx Outdated Show resolved Hide resolved
src/spinner.tsx Outdated Show resolved Hide resolved
src/confirmationModal.tsx Outdated Show resolved Hide resolved
src/confirmationModal.tsx Outdated Show resolved Hide resolved
src/confirmationModal.tsx Outdated Show resolved Hide resolved
src/styledButton.tsx Outdated Show resolved Hide resolved
src/styledButton.tsx Outdated Show resolved Hide resolved
Arnei added 5 commits May 28, 2024 16:11
Move code over to button.tsx
Optional, defaults to "currentcolor".
We should not rely on a hook in a function
working, so better let the calling component
pass it.
Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

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

A few more things with the colors. But then it's ready I think

src/button.tsx Outdated Show resolved Hide resolved
src/colors.css Outdated Show resolved Hide resolved
src/colors.css Outdated Show resolved Hide resolved
src/colors.css Outdated Show resolved Hide resolved
src/button.tsx Outdated Show resolved Hide resolved
src/button.tsx Show resolved Hide resolved
src/button.tsx Outdated Show resolved Hide resolved
@Arnei
Copy link
Member Author

Arnei commented May 29, 2024

For some of the accent colors, inverting seems somewhat tricky, as their l value is close to 50%. What do you prefer from the two images below? (Both of the "Confirm" buttons are hovered.)

Bildschirmfoto vom 2024-05-29 12-28-00
Bildschirmfoto vom 2024-05-29 12-28-44

src/colors.css Outdated Show resolved Hide resolved
src/colors.css Outdated Show resolved Hide resolved
src/button.tsx Outdated Show resolved Hide resolved
Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

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

Good to go now! Thanks again, for doing what turned out to be much more work than expected. Thanks colors!!

@LukasKalbertodt LukasKalbertodt merged commit 1b378e8 into opencast:main May 29, 2024
1 check passed
Arnei added a commit to Arnei/opencast-editor that referenced this pull request Jun 6, 2024
Fixes an issue with the upload subtitle button,
where the automatic browser dialog used by
the button would "time out", failing to open
up the subsequent file dialog. This replaces
it with a confirmation modal from appkit.

WARNING: Requires opencast/appkit#5
to be merged and released!
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