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

#6512 - Update Editor Component UI - Toolbar Lists #6605

Merged
merged 22 commits into from
Mar 6, 2024

Conversation

Miaplacidus
Copy link
Contributor

@Miaplacidus Miaplacidus commented Feb 6, 2024

Allows user to create a list without selecting text first.

Link to Issue

Closes: #6512

Description of Changes

  • allows users to add lists without selecting text

Test Plan

  • visit create thread page / create comment
  • Highlight text, make sure all list types work
  • Make sure toggling lists work
  • Make sure switching lists work as intended
  • Make sure numbered lists are in order

Other considerations

Keep in mind there is a seperate ticket for continuing a list via the enter button: #6683

@Miaplacidus Miaplacidus marked this pull request as ready for review February 6, 2024 20:44
Copy link
Contributor

@masvelio masvelio left a comment

Choose a reason for hiding this comment

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

  1. in ordered list, if you have multiple lines selected, order is not maintained
Kapture.2024-02-07.at.12.54.02.mp4
  1. for each type of list, it works only for the first entry line. Expected behaviour should be to keep using selected list type after clicking return
Kapture.2024-02-07.at.12.53.05.mp4

@Miaplacidus
Copy link
Contributor Author

@masvelio @jnaviask @CowMuon
My understanding was that the intent of the list buttons is to allow the user to start a list or quickly add a list element for them, not necessarily to create an entire list. If we want to add that as a feature, we should make a separate ticket for it.

@jessmart1213 @zakhap

@jnaviask
Copy link
Collaborator

jnaviask commented Feb 7, 2024

@Miaplacidus my understanding is that if you're inside a list, hitting enter/return should create a new list bullet automatically (which you can then backspace out of to end the list), which is fairly standard editor dynamics across the web.

@gdjohnson
Copy link
Contributor

I believe there are some Markdown style guides which prefer pre-rendered ordered lists to read as they currently do in the PR, e.g.

1. item1
1. item2
1. item3

But I do agree it can be confusing to users, especially nontechnical users/those not familiar with Markdown, who won't realize that the MD above renders normally to 1, 2, 3

@CowMuon
Copy link
Contributor

CowMuon commented Feb 8, 2024

What's left to get this in? I am not seeing any unresolved issues here, is this ready to go into v0.9.5 today? @masvelio @jnaviask @ilijabojanovic

@lzach83 lzach83 marked this pull request as draft February 8, 2024 19:54
@lzach83 lzach83 self-assigned this Feb 8, 2024
@lzach83
Copy link
Contributor

lzach83 commented Feb 8, 2024

To give an update here, the toolbar specifically the toolbar in markdown mode is not currently setup to be a "toggle" but more-so a modifier. For example if you highlight something then hit the button it'll format as a list or whatever format you desire but if you hit enter and continue. That selection is no longer present.

I'm currently working that out and just wanted the update into writing.

@CowMuon @jnaviask

@lzach83
Copy link
Contributor

lzach83 commented Feb 9, 2024

The highlighting list items and selecting the ordered list button now correctly increments the list to how many lines are highlighted.

As for @masvelio 's comment about hitting enter and it should continue the list after speaking with @CowMuon this will be a separate ticket: #6683

Reopening the ticket since the above comments have been addressed.

@lzach83 lzach83 marked this pull request as ready for review February 9, 2024 16:23
@lzach83 lzach83 requested a review from masvelio February 9, 2024 16:23
jnaviask
jnaviask previously approved these changes Feb 9, 2024
@mzparacha
Copy link
Contributor

mzparacha commented Feb 9, 2024

Are we following github markdown? If yes then clicking on any list button (check list, bullet list, numbered list) should toggle list.

Our App

Screen.Recording.2024-02-09.at.9.38.56.PM.mov

Github

Screen.Recording.2024-02-09.at.9.40.28.PM.mov

Edit: just realized that others have already mentioned this above

@mzparacha
Copy link
Contributor

mzparacha commented Feb 9, 2024

After the first list item (on the next line), if we press the numbered list button again, it starts from 1, instead of continuing the item number. I saw related comments above like

for each type of list, it works only for the first entry line. Expected behaviour should be to keep using selected list type after clicking return

but i believe this issue is different (not sure if we plan to resolve this in #6683 or resolve it at all? -- plz confirm @lzach83)

Screen.Recording.2024-02-09.at.9.43.26.PM.mov

@lzach83
Copy link
Contributor

lzach83 commented Feb 9, 2024

@mzparacha

Your first comment, good find I'll implement the fix for that now.

Second, Yeah it's because there's no context available to the toolbar so when you click that button is creates a new list rather than extending the current list. This may be solved in #6683 certainly the availability of that context will be established so it might inheritly be solved.

@lzach83
Copy link
Contributor

lzach83 commented Feb 9, 2024

@mzparacha All set on this to re-review. Thank you

@masvelio
Copy link
Contributor

@lzach83

Your first comment, good find I'll implement the fix for that now.

Is this fixed? It doesn't seem so.

Kapture.2024-02-13.at.12.47.35.mp4

Also noticed another issue that probably is a blocker for this PR. Basically if you do not highlight the list first but you want to use it, it always starts the list in the first line of the editor, regardless the cursor position.

Kapture.2024-02-13.at.12.48.39.mp4

@CowMuon CowMuon self-requested a review February 13, 2024 21:05
Copy link
Contributor

@CowMuon CowMuon left a comment

Choose a reason for hiding this comment

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

Yes! Appears to be finally ready to go in. Given the history on this story would be good to have additional eyes on this. (What did I miss!)

@lzach83 lzach83 marked this pull request as draft February 23, 2024 19:26
@lzach83 lzach83 marked this pull request as ready for review February 29, 2024 19:49
@CowMuon CowMuon self-requested a review March 4, 2024 21:07
Copy link
Collaborator

@Israellund Israellund left a comment

Choose a reason for hiding this comment

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

LGTM

@lzach83 lzach83 merged commit db38e79 into master Mar 6, 2024
7 checks passed
@lzach83 lzach83 deleted the 6512.ifu.editor-lists branch March 6, 2024 20:50
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

Successfully merging this pull request may close these issues.

Update Editor Component UI - Toolbar Lists
8 participants