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

remove In and Out from all prompts in jupyterlab #5097

Merged
merged 3 commits into from Aug 13, 2018

Conversation

@Zsailer
Copy link
Member

@Zsailer Zsailer commented Aug 10, 2018

Pinging @ellisonbg.

We talked about recovering some horizontal space in notebooks and consoles by removing the In and Out prefixes in all cell prompts.

Here, I've done the following:

  1. Removed In/Out prefixes
  2. Shrunk the prompt width from 90px to 64px.
    notebook
  3. In the light theme, I set the prompt colors of executed console cells to active prompt colors, but reduced their opacity to 0.5.
    Why? This visually separates the In vs. Out areas and communicates to the user that those cells have been executed and cannot be edited.
    light-theme
  4. In the dark theme, I set the prompt colors of executed console cells to active prompt colors, but set their opacity to 1.
    Why? I chose colors for the same reason as the point above. I did not reduce the opacity of executed cells, because it becomes too hard to see with the dark background. The dark theme visually separates the executed cells already, because active cells are a lighter color than inactive cells.
    dark-theme

This PR follows up on the conversation in #1692 about prompt design.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Aug 12, 2018

Thanks @Zsailer I have tagged this for 0.34 and will help review it.

@t-makaro
Copy link

@t-makaro t-makaro commented Aug 12, 2018

On the light theme, this might be bad for colorblind users. I'm not colorblind, but you can mimic it with color oracle.

@ellisonbg ellisonbg self-assigned this Aug 13, 2018
Copy link
Contributor

@ellisonbg ellisonbg left a comment

Overall this looks great. I have requested one minor change (remove unused commented lines) - once that is fixed, this is ready for merge. We can investigate the color-blindness situation of the dark theme in future PRs/issues (very likely there are problems)

--jp-cell-prompt-not-active-font-color: var(--md-grey-300);
--jp-cell-inprompt-font-color: var(--md-grey-50);
--jp-cell-outprompt-font-color: var(--md-grey-50);
/* --jp-cell-inprompt-font-color: var(--md-grey-50);
Copy link
Contributor

@ellisonbg ellisonbg Aug 13, 2018

@Zsailer can you remove the commented lines here?

Copy link
Member Author

@Zsailer Zsailer Aug 13, 2018

Oops! Yeah, I'll remove those now.

@Zsailer
Copy link
Member Author

@Zsailer Zsailer commented Aug 13, 2018

Thank @ellisonbg. I'll investigate why Travis and appveyor are failing, too.

@t-makaro, that's a good point. We may need to iterate on this design choice moving forward. For now, I think this is an improvement on the gray color that is used now.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Aug 13, 2018

I'm on my phone so didn't look at the logs, but CI has been flaky lately so I kicked the builds.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Aug 13, 2018

@Zsailer looks like the failure is real: the error says:

Please run jlpm run lint locally and push changes.

(a code formatting check)

@Zsailer
Copy link
Member Author

@Zsailer Zsailer commented Aug 13, 2018

Ok @ellisonbg, removed the comments and ran the linter.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Aug 13, 2018

Great, I will watch the tests and merge!

@Zsailer
Copy link
Member Author

@Zsailer Zsailer commented Aug 13, 2018

Looks like the tests passed this time--the one travis test that failed looks stochastic.

@ellisonbg ellisonbg merged commit 261a41f into jupyterlab:master Aug 13, 2018
1 of 2 checks passed
@Zsailer Zsailer mentioned this pull request Aug 14, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants