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

RFC: Ansi output options #99

Open
brism17 opened this issue Feb 22, 2022 · 2 comments
Open

RFC: Ansi output options #99

brism17 opened this issue Feb 22, 2022 · 2 comments

Comments

@brism17
Copy link

brism17 commented Feb 22, 2022

Nteract desktop app and jupyter extension currently uses the ansi-to-react library that takes four props to configure the ansi outputs. I want to configure the passing of optional options to the ansi object the controls stream and error outputs.

Motivation

Azure notebooks needs to change some of the default ansi values to fit our customers' needs. We understand that what is best for our product might not be what is best for other consumers of nteract. Making these options configurable will help all nteract consumers create the best experience to fit their needs.

Goals:

  • Azure notebooks wants to make links in error outputs clickable.
  • Azure notebooks wants to improve the accessibility of ansi outputs foreground colors to make certain outputs more visible in our themes.

Design and Implementation

Ansi to react has four properties that are passed down as props found here

 declare interface Props {
        children?: string;
        linkify?: boolean;
        className?: string;
      useClasses?: boolean;
  }

I want to add two of these options: useClasses and Linkify, to be passed down via the kernel-output-error class and the stream-text.tsx so that individual consumers have the power to control how ansi outputs are rendered. These options will be added to the existing props for these output classes.
The following is kernel-ouput-error:

   interface Props {
     className?: string;
     output: ImmutableErrorOutput;
     output_type: "error";
     linkify?: boolean;
     useClasses?: boolean;
  }

The following is stream-text:

  interface Props {
      output_type: "stream";
      output: ImmutableStreamOutput;
      linkify?: boolean;
      useClasses?: boolean;
   }

These options would override the default properties if provided which would match the current options in nteract.

Testing

To validate the correct behavior of the new ansi options I will add unit tests for the different options configurations and for if no options are provided..

Compatibility

If no options are provided I will provide default values that are the same as the current settings in nteract.

User Impact

No user impact if a user doesn't pass down options, the current behavior will stay the same. However, if a user chooses to pass down options they will have full control of how ansi outputs are rendered.

@captainsafia captainsafia transferred this issue from nteract/nteract Mar 1, 2022
@captainsafia
Copy link
Member

Azure notebooks wants to make links in error outputs clickable.

Hm. Does this not work at the moment or would you like to be able to conditionally enable or disable it?

These options will be added to the existing props for these output classes.

Is there a chance we can add them under a new AnsiOptions option so that we have the ability to manage this type independently?

Where are AnsiOptions stored? In the app state?

@brism17
Copy link
Author

brism17 commented Mar 1, 2022

Azure notebooks wants to make links in error outputs clickable.

This doesn't work at the moment and the goal of this RFC is to be able to conditionally enable it. The linkify option for ansi is set to false here. Instead of false being hard coded it would read from a prop value passed down through kernel-output-error by nteract's consumers.

Is there a chance we can add them under a new AnsiOptions option so that we have the ability to manage this type independently?

I thought about doing this initially but since there were only two props I wanted to add I thought it would be fine to just add them to the list of existing props for stream-text and kernel-output-error. The options are just stored as props in the react object and passed down to the Ansi class when its initialized. There is no reason for these options to need to be stored in state. Another reason why I didn't make an ansiOptions prop is because kernel-output-error already has one of the ansi props (className) that it passes down to the ansi object. I don't want to add any breaking changes and it wouldnt make sense for one of the ansi props to be outside of the ansiOptions object to make this change .

@captainsafia captainsafia transferred this issue from nteract/ansi-to-react Mar 7, 2022
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

No branches or pull requests

2 participants