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

[GSoC 2024] Use toolkit button for all buttons: Dialog, extension manager, notification, running tabs.updated files #16438

Draft
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

Mehak261124
Copy link
Collaborator

Issue:
Inconsistencies have arisen in the user interface across various JupyterLab extensions, including the Dialog, Extension Manager, Notification Center, and Running Tabs.

Background:
The aim of this update is to standardize the UI elements across JupyterLab by utilizing the Button component from the Jupyter Toolkit. The Jupyter Toolkit's components are built on the Fast Foundation's Open UI system and are designed to provide a consistent look and enhanced accessibility across web applications.

The Button component was specifically integrated into several core components and extensions of JupyterLab to align with modern design practices and ensure UI consistency. Changes were made in the following files:

widget.tsx: Updated the Extension Manager's buttons to utilize the Button component from the Jupyter Toolkit, replacing previous implementations. This change ensures that all buttons within the Extension Manager now have a uniform appearance and behavior.
dialog.tsx: Employed Button in dialog actions, replacing previous HTML button elements. This standardizes dialog interactions throughout the application.
notificationplugin.tsx: Applied Button in notification actions to unify notification interaction points, ensuring consistent user experience across various notifications.
These updates help streamline component maintenance, improve stylistic consistency, and upgrade the overall user experience.

Screenshots:
Extensionmanager Button Before
Before

Extensionmanager Button After
After

Notification Button Before
_before

Notification Button After
_after

Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @Mehak261124

This looks much cleaner.

To solve some of the outstanding issues on the CI, could you run locally the following command and commit the changes:

jlpm integrity

I spotted two cases were you did not switch to the toolkit button.

  • The toast close button at

<button
className={`jp-Button jp-mod-minimal ${TOAST_CLOSE_BUTTON_CLASS}${
props.closeIconMargin ? ` ${TOAST_CLOSE_BUTTON_MARGIN_CLASS}` : ''
}`}
title={props.title ?? ''}
onClick={props.close}
>
<props.closeIcon className="jp-icon-hover" tag="span" />
</button>

It should be something like that:

      <Button
        className={`jp-Button jp-icon-hover ${TOAST_CLOSE_BUTTON_CLASS}${
          props.closeIconMargin ? ` ${TOAST_CLOSE_BUTTON_MARGIN_CLASS}` : ''
        }`}
        title={props.title ?? ''}
        onClick={props.close}
        minimal
      >
        <props.closeIcon tag={null} />
      </Button>
  • The footer button of the dialog:

const e = document.createElement('button');
e.className = this.createItemClass(button);

It should be const e = document.createElement('jp-button');

Then there is the issue with createItemClass because we should not rely on the class any longer but on the attribute appearance.
So instead of applying jp-mod-accept, the appearance should be set to accent. jp-mod-reject matches appearance="neutral" and jp-mod-warn is appearance="error".

I'm gonna work on the toolkit to add a new attribute on the button called size that should help adapting the button and be less restrictive that the minimal attribute. It is a more common pattern available in other toolkit.


I changed slightly the title of this PR as I saw a couple of buttons won't be changed by it (you can look for <button to find them but that could be done in a follow up PR).

Comment on lines 23 to 26
/**
* Button content
*/
value?: string;
Copy link
Member

Choose a reason for hiding this comment

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

This is unused. The content should be automatically available through the children prop (see the documentation for more info).

Suggested change
/**
* Button content
*/
value?: string;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it is available but here i used toolkit button component as per interface its value?: string; and in the button component as per interface its value?: string | ReadonlyArray<string> | number | undefined;

So for uniformity i added value?: string.

Comment on lines 35 to 40
className={classes(
props.className,
minimal ? 'jp-mod-minimal' : '',
small ? 'jp-mod-small' : '',
'jp-Button'
)}
className={classes(className, small ? 'jp-mod-small' : '', 'jp-Button')}
Copy link
Member

Choose a reason for hiding this comment

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

Let's not change that as consumer of the Button may use those classes to tune the styling of the button.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just removed the minimal here as the toolkit button has the minimal attribute so i used the toolkit button's minimal attribute if that is not expected i will change to the old class.

packages/apputils/src/dialog.tsx Show resolved Hide resolved
@Mehak261124
Copy link
Collaborator Author

Hey @fcollonval I ran this jlpm integrity command and get the output Repo integrity verified!.

@Mehak261124
Copy link
Collaborator Author

Mehak261124 commented Jun 6, 2024

Thanks @Mehak261124

This looks much cleaner.

To solve some of the outstanding issues on the CI, could you run locally the following command and commit the changes:

jlpm integrity

I spotted two cases were you did not switch to the toolkit button.

  • The toast close button at

<button
className={`jp-Button jp-mod-minimal ${TOAST_CLOSE_BUTTON_CLASS}${
props.closeIconMargin ? ` ${TOAST_CLOSE_BUTTON_MARGIN_CLASS}` : ''
}`}
title={props.title ?? ''}
onClick={props.close}
>
<props.closeIcon className="jp-icon-hover" tag="span" />
</button>

It should be something like that:

      <Button
        className={`jp-Button jp-icon-hover ${TOAST_CLOSE_BUTTON_CLASS}${
          props.closeIconMargin ? ` ${TOAST_CLOSE_BUTTON_MARGIN_CLASS}` : ''
        }`}
        title={props.title ?? ''}
        onClick={props.close}
        minimal
      >
        <props.closeIcon tag={null} />
      </Button>
  • The footer button of the dialog:

const e = document.createElement('button');
e.className = this.createItemClass(button);

It should be const e = document.createElement('jp-button');

Then there is the issue with createItemClass because we should not rely on the class any longer but on the attribute appearance. So instead of applying jp-mod-accept, the appearance should be set to accent. jp-mod-reject matches appearance="neutral" and jp-mod-warn is appearance="error".

I'm gonna work on the toolkit to add a new attribute on the button called size that should help adapting the button and be less restrictive that the minimal attribute. It is a more common pattern available in other toolkit.

I changed slightly the title of this PR as I saw a couple of buttons won't be changed by it (you can look for <button to find them but that could be done in a follow up PR).

Hey @fcollonval I am trying to figure out how can i change the button being created through js code into button component in our toolkit. If possible can you provide any documentation that can help me as I am facing difficulty around interfaces.

@krassowski
Copy link
Member

If the underlying element is jp-button rather than button it has a potential to break some downstream extensions, CI automatons, and themes. Should we talk about whether this is 4.3 or 5.0 item? We already went through this in the toolbars change and it was slightly annoying but as buttons in general have a larger surface I think we need to be a bit more careful and think if there are any mitigations that could be applied.

@fcollonval
Copy link
Member

please bot update snapshots

@fcollonval fcollonval changed the title Use toolkit button for all buttons: Dialog, extension manager, notification, running tabs.updated files [GSoC 2024] Use toolkit button for all buttons: Dialog, extension manager, notification, running tabs.updated files Jun 19, 2024
@fcollonval
Copy link
Member

@Mehak261124 we discuss about this PR at the weekly call. To limit the impact on downstream extensions, the Button in the ui-components package will be kept as is and deprecated. So it means we can move forward with this one. The first step you will need to do is to upgrade @jupyter/web-components and @jupyter/react-components to use the latest version 0.16.1.

@krassowski krassowski modified the milestones: 4.3.0, 4.4.0 Aug 9, 2024
@krassowski
Copy link
Member

Since this is still in draft I bumped the milestone to 4.4.0. If it is already ready or will be ready very soon, we can move it back to 4.3.0

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.

3 participants