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 AlertComponent for displaying alerts in the GUI #1975

Open
wants to merge 31 commits into
base: develop
Choose a base branch
from

Conversation

Rishi-0007
Copy link
Contributor

@Rishi-0007 Rishi-0007 commented Oct 8, 2024

This pull request introduces the Alert.tsx to the Taipy framework and adds relevant tests. The component allows rendering of customizable alerts with dynamic message, severity, and variant properties. Additionally, there have been significant changes to existing components as per the maintainers' request.

This PR addresses the issue outlined in feature #693.

Changes in this PR:

  • Added Alert.tsx: A new component that renders an alert using Material-UI's Alert component.
  • Renamed Components: As requested by the maintainers, the existing Alert.tsx and Alert.spec.tsx have been renamed to Notification.tsx and Notification.spec.tsx to avoid naming conflicts.
  • Modified factory.py: Registered the Alert Component with properties (message, severity, variant) and enabled dynamic updates.
  • Updated viselements.json: Added the definition for the Alert Component, including its properties.
  • Added tests: Created new tests for the Alert Component to dynamically verify its behavior for different alert messages, severities, and variants.
  • All Tests Passing: Ensured that all existing and new tests are passing successfully.

Example test:

doc/gui/examples/Alert.py:

import random
from taipy.gui import Gui

# Initial dynamic properties
severity = "error"
variant = "filled"
message = "This is an error message."
render = True

# A simple page that binds the Alert component to dynamic variables and adds a button to trigger the update
page = """
<|{message}|alert|severity={severity}|variant={variant}|render={render}|>
<br/>
<|Update Alert|button|on_action=update_alert|>
"""

# Function to toggle between variants and severities
def update_alert(state):
    severities = ["error", "warning", "info", "success"]
    variants = ["filled", "outlined"]
    
    state.severity = random.choice(severities)
    state.variant = random.choice(variants)
    state.message = f"This is a {state.severity} message with {state.variant} variant."

if __name__ == "__main__":
    gui = Gui(page)
    gui.run(title="Test Alert")

Output:

Screencast.from.2024-10-16.01-04-41.mp4

Additional Information:

  • Followed the coding standards and branch naming conventions.
  • All tests have passed, and the component is working as expected.

Checklist:

  • The code follows the Taipy coding style.
  • Tests are included.
  • Code is rebased and up to date with the latest develop branch.
  • Pre-commit hooks are passing without errors.
  • All Taipy tests pass successfully.

Related Issue:

This PR resolves issue #693.

@Rishi-0007 Rishi-0007 mentioned this pull request Oct 8, 2024
4 tasks
@Rishi-0007
Copy link
Contributor Author

@quest-bot loot #693

@quest-bot quest-bot bot added the ⚔️ Quest Tracks quest-bot quests label Oct 8, 2024
Copy link

quest-bot bot commented Oct 8, 2024

Quest PR submitted! image Quest PR submitted!

@Rishi-0007, you are attempting to solve the issue and loot this Quest. Will you be successful?


Questions? Check out the docs.

@jrobinAV jrobinAV added 🖰 GUI Related to GUI 🟨 Priority: Medium Not blocking but should be addressed hacktoberfest hacktoberfest issues hacktoberfest - 100💎 Issues rewarded by 100 points labels Oct 8, 2024
frontend/taipy-gui/src/components/Taipy/AlertComponent.tsx Outdated Show resolved Hide resolved
frontend/taipy-gui/src/components/Taipy/AlertComponent.tsx Outdated Show resolved Hide resolved
frontend/taipy-gui/src/components/Taipy/AlertComponent.tsx Outdated Show resolved Hide resolved
frontend/taipy-gui/src/components/Taipy/AlertComponent.tsx Outdated Show resolved Hide resolved
frontend/taipy-gui/src/components/Taipy/index.ts Outdated Show resolved Hide resolved
taipy/gui/_renderers/factory.py Outdated Show resolved Hide resolved
frontend/taipy-gui/src/components/Taipy/test-alert.py Outdated Show resolved Hide resolved
@Rishi-0007
Copy link
Contributor Author

Hi @FredLL-Avaiga ,
Please review the changes when you get a chance. Let me know if any further adjustments are needed.

@jrobinAV
Copy link
Member

jrobinAV commented Oct 9, 2024

@Rishi-0007 If you have a screenshot (or even a few) to illustrate your proposal, that would make the review easier.

In any case, thank you for your help.

taipy/gui/_renderers/factory.py Outdated Show resolved Hide resolved
taipy/gui/viselements.json Outdated Show resolved Hide resolved
@Rishi-0007
Copy link
Contributor Author

Hi @FredLL-Avaiga,
Please review the latest changes and let me know if any further adjustments are needed.

@Rishi-0007
Copy link
Contributor Author

Hey @FredLL-Avaiga ,

I've updated the message property in viselements.json as requested, describing it as a dynamic string. Apologies for the earlier oversight. Please review the changes when you have a moment.

Copy link
Member

@namnguyen20999 namnguyen20999 left a comment

Choose a reason for hiding this comment

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

almost there

taipy/gui/viselements.json Outdated Show resolved Hide resolved
taipy/gui/_renderers/factory.py Outdated Show resolved Hide resolved
frontend/taipy-gui/src/components/Taipy/Notification.tsx Outdated Show resolved Hide resolved
frontend/taipy-gui/src/components/Taipy/Notification.tsx Outdated Show resolved Hide resolved
@FredLL-Avaiga
Copy link
Member

Hi @Rishi-0007, can you fix the conflicts and resolve the conversation that are no longer active ?

@Rishi-0007
Copy link
Contributor Author

Hi @FredLL-Avaiga,

Thank you for your feedback. I'm not entirely sure about the best way to resolve the conflicts and manage the conversations that are no longer active. Could you kindly provide some guidance on how to proceed with these tasks?

I appreciate your help!

severity = "error"
variant = "filled"
message = "This is an error message."
render = True
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't use render here

# A simple page that binds the Alert component to dynamic variables and adds a button to trigger the update
page = """
<|{message}|alert|severity={severity}|variant={variant}|render={render}|>
<br/>
Copy link
Member

Choose a reason for hiding this comment

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

no need for br or button

<|Update Alert|button|on_action=update_alert|>
"""

# Function to toggle between variants and severities
Copy link
Member

Choose a reason for hiding this comment

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

no need for a button on_action

doc/gui/examples/Alert.py Show resolved Hide resolved
@Rishi-0007
Copy link
Contributor Author

@FredLL-Avaiga does this looks good to you?

from taipy.gui import Gui

severity = "error"
variant = "filled"
message = "This is an error message."

page = """
<|{message}|alert|severity={severity}|variant={variant}|>
"""

if __name__ == "__main__":
    gui = Gui(page)
    gui.run(title="Test Alert")

@FredLL-Avaiga
Copy link
Member

You could even be not simplistic and keep only the message as a variable and group the 2 lines with gui

@Rishi-0007
Copy link
Contributor Author

You could even be not simplistic and keep only the message as a variable and group the 2 lines with gui

from taipy.gui import Gui

message = "This is an error message."

Gui(f"<|{{message}}|alert|severity='error'|variant='filled'|>").run(title="Test Alert")

Is this gd?

@Rishi-0007 Rishi-0007 force-pushed the feature/#693-AddAlertVisualElement branch from bdb722c to 354eae0 Compare October 19, 2024 17:56
@FredLL-Avaiga
Copy link
Member

You could even be not simplistic and keep only the message as a variable and group the 2 lines with gui

from taipy.gui import Gui

message = "This is an error message."

Gui(f"<|{{message}}|alert|severity='error'|variant='filled'|>").run(title="Test Alert")

Is this gd?

A bit too compact

@Rishi-0007 Rishi-0007 force-pushed the feature/#693-AddAlertVisualElement branch from 4fc9a24 to d4fec0e Compare October 19, 2024 21:22
- Refactor Alert.py example to remove unused code and simplify the page structure.
- Add package.json file for frontend/taipy directory from develop branch.
@Rishi-0007
Copy link
Contributor Author

Hey @FredLL-Avaiga ,
I've added a simple test :

from taipy.gui import Gui

severity = "error"
variant = "filled"
message = "This is an error message."

page = """
<|{message}|alert|severity={severity}|variant={variant}|>
"""

if __name__ == "__main__":
    gui = Gui(page)
    gui.run(title="Test Alert")

output:
Screenshot from 2024-10-20 02-36-32

Also i have removed package.json from my commit.
Thank you for your time. I hope everything is resolved now.

Copy link
Member

@FredLL-Avaiga FredLL-Avaiga left a comment

Choose a reason for hiding this comment

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

I'd like you to NOT commit any package.json file

Copy link
Member

Choose a reason for hiding this comment

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

I'd like you to NOT commit this file

Copy link
Member

@FredLL-Avaiga FredLL-Avaiga left a comment

Choose a reason for hiding this comment

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

Almost there

alerts: AlertMessage[];
}

const TaipyNotification = ({ alerts }: NotificationProps) => {
Copy link
Member

Choose a reason for hiding this comment

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

?

@FredLL-Avaiga FredLL-Avaiga self-requested a review October 21, 2024 16:35
@FredLL-Avaiga
Copy link
Member

Waiting for this PR to be merged because it'll be easier to integrate then

@Rishi-0007
Copy link
Contributor Author

Waiting for this PR to be merged because it'll be easier to integrate then

Thanks for the update! I understand, and I’ll wait for the PR to be merged.

@FredLL-Avaiga
Copy link
Member

The PR about closing notification (that has an impact on the old alert component) was merged.
I'm sorry but can you manage the merge and conflicts resolution ?
tx @Rishi-0007

Copy link
Member

@FredLL-Avaiga FredLL-Avaiga left a comment

Choose a reason for hiding this comment

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

all good except for the conflicts with the last PR merged

if (alert) {
if (alert.atype === "") {
if (lastKey.current) {
closeSnackbar(lastKey.current);
Copy link
Member

@FredLL-Avaiga FredLL-Avaiga Oct 23, 2024

Choose a reason for hiding this comment

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

Something was lost in the conflict resolution
this should be

    useEffect(() => {
        if (alert) {
             const notificationId = alert.notificationId || "";
            if (alert.atype === "") {
                closeSnackbar(notificationId);  
            } else {
                enqueueSnackbar(alert.message, {
                    variant: alert.atype as VariantType,
                    action: notifAction,  
                    autoHideDuration: alert.duration,
                    key: notificationId,  
                });
                alert.system && new Notification(document.title || "Taipy", { body: alert.message, icon: faviconUrl });
            }
            dispatch(createDeleteAlertAction(notificationId));
        }
    }, [alert, enqueueSnackbar, closeSnackbar, notifAction, faviconUrl, dispatch]);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🖰 GUI Related to GUI hacktoberfest - 100💎 Issues rewarded by 100 points hacktoberfest hacktoberfest issues 🟨 Priority: Medium Not blocking but should be addressed ⚔️ Quest Tracks quest-bot quests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants