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.

Sign up for GitHub

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

Jump to bottom

fix: add experimental note to webgl setting #149

Merged
merged 1 commit into from
Aug 28, 2020
Merged

fix: add experimental note to webgl setting #149

merged 1 commit into from
Aug 28, 2020

Conversation

UziTech
Copy link
Member

@UziTech UziTech commented Aug 25, 2020 •
edited
Loading

image

@UziTech UziTech mentioned this pull request Aug 25, 2020
Closed
@UziTech UziTech requested a review from the-j0k3r August 25, 2020 14:45
the-j0k3r
the-j0k3r requested changes Aug 25, 2020
View reviewed changes
src/config.js Outdated Show resolved Hide resolved
@UziTech UziTech force-pushed the webgl-experimental branch from e0fc18a to c435eba Compare August 25, 2020 16:25
@UziTech UziTech added the documentation 📝 Improvements or additions to documentation label Aug 25, 2020
the-j0k3r
the-j0k3r reviewed Aug 25, 2020
View reviewed changes
src/config.js Outdated
@@ -254,7 +254,7 @@ export const config = configOrder({
type: 'object',
properties: {
webgl: {
title: 'WebGL Renderer',
title: 'WebGL Renderer ⚠️EXPERIMENTAL⚠️',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
title: 'WebGL Renderer ⚠️EXPERIMENTAL⚠️',
title: '⚠️ Experimental WebGL Renderer',

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Again is the caution on Experimental or WebGL. Keep in mind that some people (probably most people) won't know what WebGL is.

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 also want it to convey that the renderer is experimental not WebGL.

Copy link
Member

@the-j0k3r the-j0k3r Aug 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I also want it to convey that the renderer is experimental not WebGL.

Then neither is good for that.

The description is the way to go for that not the title. For that we need.

`description: '⚠️ Enable the WebGL-based renderer using the experimental xterm.js [WebGL addon](https://github.com/xtermjs/xterm.js/tree/master/addons/xterm-addon-webgl) ',

Now that conveys the message clearly

Copy link
Member Author

Choose a reason for hiding this comment

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

With the caution symbol at the beginning I don't know if you are saying "Hey, you might want to enable me!" or "Hey, you might not want to enable me!" because it is not clear that the reason for the symbol is because it is experimental.

Copy link
Member Author

Choose a reason for hiding this comment

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

or what about adding a short description at the end.

image

Copy link
Member

Choose a reason for hiding this comment

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

You are creating two sentences where one was and making the pertinent word all the way to the end of first sentence.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why is it bad to have more than one sentence? And when people skim something they tend to remember the beginning and the end more than the middle.

Copy link
Member Author

@UziTech UziTech Aug 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This seems like a good way to let people know why they might want to enable it and why they might not.

Copy link
Member

Choose a reason for hiding this comment

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

As is now is fine =)

@UziTech UziTech force-pushed the webgl-experimental branch from c435eba to 1a36b3a Compare August 28, 2020 13:38
@the-j0k3r the-j0k3r self-requested a review August 28, 2020 15:47
the-j0k3r
the-j0k3r approved these changes Aug 28, 2020
View reviewed changes
Copy link
Member

@the-j0k3r the-j0k3r left a comment

Choose a reason for hiding this comment

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

as is

@UziTech UziTech merged commit c1b5eae into master Aug 28, 2020
@UziTech UziTech deleted the webgl-experimental branch August 28, 2020 16:23
@UziTech
Copy link
Member Author

UziTech commented Aug 28, 2020

I guess it is a little better than it was before but I still think a lot of people will not notice it.

@github-actions
Copy link

🎉 This PR is included in version 9.1.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot added the released 📮 Release has been made label Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@the-j0k3r the-j0k3r the-j0k3r approved these changes

Assignees
No one assigned
Labels
documentation 📝 Improvements or additions to documentation released 📮 Release has been made
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@UziTech @the-j0k3r