Skip to content

Add message formatting tips help menu#1025

Merged
neiljp merged 6 commits intozulip:mainfrom
Ezio-Sarthak:markdown-help-popup
Jul 30, 2021
Merged

Add message formatting tips help menu#1025
neiljp merged 6 commits intozulip:mainfrom
Ezio-Sarthak:markdown-help-popup

Conversation

@Ezio-Sarthak
Copy link
Member

@Ezio-Sarthak Ezio-Sarthak commented May 9, 2021

What does this PR do?!

Commit flow

  1. Add hotkey meta m to show markdown popup.
  2. Create a new file markdown_examples.py containing markdown help data.
  3. refactor: Amend the type annotation for PopUpView contents.
  4. Structure UI in MarkdownHeloView class.
  5. Add UI elements to markdown help view on toggling.
  6. Add support for accessing the popup from compose area.

Doubts/Concerns

  • The Popup name, i.e., MarkdownHelpView could be replaced with one having message formatting instead?.
  • The markdown help data might be migrated to ui_mappings

Screenshot(s)/GIF(s)
improved_header

@zulipbot zulipbot added size: XL [Automatic label added by zulipbot] enhancement New feature or request labels May 9, 2021
@mkp6781
Copy link
Contributor

mkp6781 commented May 10, 2021

@Ezio-Sarthak Thanks for working on this markdown help menu. This will be really helpful for those who are new to markdown.

I have left a few comments inline requesting minor changes. Also, in the final commit's title you can mention ui.py as a changed file.

@Ezio-Sarthak Ezio-Sarthak force-pushed the markdown-help-popup branch from eeebda7 to ba3259c Compare May 10, 2021 15:23
@Ezio-Sarthak
Copy link
Member Author

@mkp6781 Thanks for the review! Updated the PR addressing the points :)

@Ezio-Sarthak Ezio-Sarthak force-pushed the markdown-help-popup branch from ba3259c to b234195 Compare May 10, 2021 15:29
Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@Ezio-Sarthak This is a pretty useful feature as it stands; I've not tested it but it looks great in the screenshot 👍

My concern is that the table titles scroll too, which is a little confusing, particularly if you have a short screen (height). That could be for the next version of this feature though.

I'm not sure of the best place for the markdown data; my gut feeling is that it is more internal to the software than symbols/keys/styles (which are definitely configuration, even if default), but perhaps big enough to have its own file.

This is awaiting a black rebase from what I can tell.

@neiljp neiljp added missing feature: user A missing feature for all users, present in another Zulip client PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed enhancement New feature or request labels Jun 3, 2021
@neiljp
Copy link
Collaborator

neiljp commented Jun 3, 2021

Like with the webapp, we could potentially simplify the UI at a later stage by using tabs or similar to make a more universal help menu - which is a good example of how having a general tabbing or similar widget would be useful, not just for large/raw message rendering 👍

@Ezio-Sarthak Ezio-Sarthak force-pushed the markdown-help-popup branch from b234195 to c015b34 Compare July 21, 2021 03:54
@Ezio-Sarthak Ezio-Sarthak added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jul 21, 2021
@neiljp
Copy link
Collaborator

neiljp commented Jul 21, 2021

@Ezio-Sarthak I've not looked at the code but I confirmed the headers are nicely fixed now, though we might benefit from styling them more befitting a title - maybe the same as popup categories?

Are the set in markdown.py just a selection you wanted to try, or did you pick them specifically? This can be extended/adjusted later of course :)

This has 'help' for each element; did you plan to extend the table later to have a name (that's what it is right now?) as well as maybe 'notes' (eg. emoji are actually shown in the webapp, italic is shown as bold, ...)?

@neiljp neiljp added feedback wanted and removed PR needs review PR requires feedback to proceed labels Jul 21, 2021
@Ezio-Sarthak
Copy link
Member Author

@neiljp thanks for giving this a go! I agree the header may be made to stand out more. I'll use the popup_category palette for this 👍.

Re the set in markdown.py, I just saw what all elements are shown in web app's message formatting tips menu, and listed them in order, ensuring we support that element in ZT as well :)

Re the help, partially yes. I initially kept it to let devs understand what a particular dict is about in markdown.py(I can document this by comments as well :))

@Ezio-Sarthak Ezio-Sarthak force-pushed the markdown-help-popup branch 3 times, most recently from 14c9c47 to abeeab1 Compare July 23, 2021 16:11
Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@Ezio-Sarthak This is getting close 👍

It was good to investigate the user-customization aspects, but I think this can be much simpler including in the data structures, which I think you could almost revert to the previous version - it's fine to include the extra 'name' fields, and I think we should file an issue for showing the 'name' or some 'notes' like in the web app.

Note that /me dances makes sense to use the current user, but we likely want a different user for other mentions, as that's the typical use-case.

If we can use a Union between the existing content type and that provided by the soup2markup method then this seems more explicit than just using Any?

Could the window could be wider? Were you restricting the width for a specific reason?

How feasible is this to access from compose?

Minor: maybe markdown_examples.py would be more specific?

@neiljp neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Jul 23, 2021
@Ezio-Sarthak Ezio-Sarthak force-pushed the markdown-help-popup branch from abeeab1 to 7e55290 Compare July 25, 2021 14:02
Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@Ezio-Sarthak Thanks for addressing most of the points! It's useful we'll be able to access this from compose now :)

This looks close, with as you say the type and width calculation being the sticking points. If we can make the minimal change I suggested in type and add a FIXME referencing a new issue for improving soup2markup typing then I don't see any issue with this as a v1 👍

The rebase doesn't look too tricky.

@Ezio-Sarthak Ezio-Sarthak force-pushed the markdown-help-popup branch 2 times, most recently from e566679 to 562d60d Compare July 30, 2021 04:09
This commit adds a new config file to centralize markdown-related
examples.

Each example markdown element's data is required to include:
 * `name` that is a help text for a given markdown element. It could be
   further extended to something like "notes" for a markdown element.
 * `raw_text` that would be used (as syntax) in markdown input fields,
 * `html_element` that stores the HTML content, which helps conversion
    from HTML -> urwid via `soup2markup`.
This short commit updates the type annotation for the contents of
PopUpView. This is done so that there isn't a conflict in follow-up
commits where the table content would contain types such as
Tuple[str, Any]. Currently the tuple types supported are only
Tuple[str, str].

Note that `Any` here is not very explicit and clear, and so potentially
needs improvement. A FIXME is added noting the same.
@Ezio-Sarthak Ezio-Sarthak added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jul 30, 2021
This commit adds the class responsible to show markdown help. It
fetches markdown data from the newly created `markdown_examples.py`.

Tests added for MarkdownHelpView.
This commit adds:
 * member function show_markdown_help() to trigger the popup.
 * keypress event in `ui.py` to trigger the popup.

Fixes zulip#623.
This commit adds a keypress event to show the markdown help menu while
a user is in the compose area, to help users to check for proper
markdown syntax while writing a message.

This is a fuller fix to zulip#623.

Test added.
@neiljp neiljp force-pushed the markdown-help-popup branch from 562d60d to 88bd743 Compare July 30, 2021 05:18
@neiljp
Copy link
Collaborator

neiljp commented Jul 30, 2021

@Ezio-Sarthak I made a few minor textual changes but otherwise am merging as you had it - thanks! 🎉
(I do like that the examples are easily customizable in the config file 👍)

@neiljp neiljp merged commit b5ac52e into zulip:main Jul 30, 2021
@neiljp neiljp added this to the Next Release milestone Jul 30, 2021
@neiljp neiljp added area: UI General user interface update and removed PR needs review PR requires feedback to proceed feedback wanted labels Jul 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: UI General user interface update missing feature: user A missing feature for all users, present in another Zulip client size: XL [Automatic label added by zulipbot]

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Show message formatting tips

4 participants