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 floating snackbar #6683

Merged
merged 13 commits into from
Jul 11, 2022
Merged

Conversation

alexander-schranz
Copy link
Member

@alexander-schranz alexander-schranz commented Jul 6, 2022

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Fixed tickets fixes #
Related issues/PRs #
License MIT
Documentation PR sulu/sulu-docs#

What's in this PR?

Add floating snackbar.

snackbar

Why?

To show snackbar messages for copy and pasting of blocks.

To Do

  • Implement Snackbar Container
  • Implement block messages

@alexander-schranz alexander-schranz added the Feature New functionality not yet included in Sulu label Jul 6, 2022
@alexander-schranz alexander-schranz force-pushed the feature/snackbar branch 3 times, most recently from 09a58fd to c0a4b02 Compare July 7, 2022 15:24
@alexander-schranz alexander-schranz marked this pull request as ready for review July 8, 2022 09:18
Comment on lines +64 to +65
"sulu_admin.info": "Info",
"sulu_admin.success": "Success",
Copy link
Contributor

Choose a reason for hiding this comment

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

Those are missing in admin.de.json, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@@ -24,6 +24,7 @@ type Props<T: string, U: {type: T}> = {|
onChange: (value: Array<U>) => void,
onSettingsClick?: (index: number) => void,
onSortEnd?: (oldIndex: number, newIndex: number) => void,
onTriggerMessage?: (message: Message) => void,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think trigger is the correct term for this use case, but I also can't find another one that fits better ...

Copy link
Member Author

Choose a reason for hiding this comment

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

I even didnt wanted to use "on" here but our eslint rule doesnt allow it. Alternative to trigger would be onDispatchMessage. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

i think would go with onShowSnackbar or onDisplaySnackbar

Copy link
Member Author

Choose a reason for hiding this comment

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

I would go with onDisplaySnackbar then @luca-rath what do you think?

@matthieu2607
Copy link

nice feature ;)

@@ -1,6 +1,6 @@
// @flow
import React from 'react';
import classNames from 'classnames';
import classnames from 'classnames';
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we imported this as classNames in all other componenets. not sure if there is a reason, but i think i would keep things consistent 🙈

</div>
```

There is also a behaviour `floating` option used for floating snackbar:
Copy link
Contributor

Choose a reason for hiding this comment

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

this is already described this above right? 🤔

@@ -0,0 +1,13 @@
.container {
Copy link
Contributor

Choose a reason for hiding this comment

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

do you think we can reuse this somewhere? if this is specific to the <Application>, i think i would not extract a separate component

Copy link
Member Author

Choose a reason for hiding this comment

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

yes inside an overlay that is way I created a component for it. While it is currented decided not to implement it, I would keep the component here.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is general style and the application is handling when it is used in the application context.

@@ -24,6 +24,7 @@ type Props<T: string, U: {type: T}> = {|
onChange: (value: Array<U>) => void,
onSettingsClick?: (index: number) => void,
onSortEnd?: (oldIndex: number, newIndex: number) => void,
onTriggerMessage?: (message: Message) => void,
Copy link
Contributor

Choose a reason for hiding this comment

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

i think would go with onShowSnackbar or onDisplaySnackbar


type Props = {|
behaviour: 'static' | 'floating',
Copy link
Contributor

Choose a reason for hiding this comment

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

this prop does not really change any behaviour, right? 🙈

Copy link
Contributor

Choose a reason for hiding this comment

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

it affects the style and the text, right? i think we have skin on a few other components. think that would describe it better

Copy link
Member Author

Choose a reason for hiding this comment

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

Is fine for me, was also unsure about it.

Copy link
Contributor

@niklasnatter niklasnatter left a comment

Choose a reason for hiding this comment

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

I am sorry, i did not see the text/message inconsistency before 😕

if (onDisplaySnackbar) {
onDisplaySnackbar({
type: 'info',
text: translate('sulu_admin.%count%_blocks_pasted', {count: newElements.length}),
Copy link
Contributor

Choose a reason for hiding this comment

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

think i would call this message as it is also called message on the Snackbar and in the Overlay

@@ -10,3 +10,9 @@ export type RenderBlockContentCallback<T: string, U: {type: T}>
= (value: U, type: T, index: number, expanded: boolean) => Node;

export type BlockMode = 'static' | 'sortable' | 'selectable';

export type Message = {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe call this SnackbarMessage

// @flow
export type Message = {
icon?: string,
text: string,
Copy link
Contributor

Choose a reason for hiding this comment

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

think i would call this message as it is also called message on the Snackbar and in the Overlay

Copy link
Member Author

Choose a reason for hiding this comment

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

find message.message strange.

Copy link
Member Author

Choose a reason for hiding this comment

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

as discussed we will curently keep message.text maybe in future we should migrate all components using the message object instead have separate props for every thing. This effects the Snackbar, but also Dialog and maybe overlays which has a snackbarMessage and snackbarType.

@niklasnatter niklasnatter merged commit 5f42000 into sulu:2.5 Jul 11, 2022
@alexander-schranz alexander-schranz deleted the feature/snackbar branch July 11, 2022 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New functionality not yet included in Sulu
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants