-
-
Notifications
You must be signed in to change notification settings - Fork 381
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
Rich text spans #1589
base: master
Are you sure you want to change the base?
Rich text spans #1589
Conversation
541d801
to
167b902
Compare
Sorry it's taken me so long to get to this! I have a first round of QA feedback that it'd be most awesome if you could help polish it up for being production-ready.
Sorry if that's an overwhelming bunch of things! Feel free to push back on any that you'd prefer to de-scope for follow-up work after an earlier merge. I was erring on the side of thoroughness for this first QA pass. Thanks :) |
Reopening since I'm planning to either make my requested changes above or enlist community contributors. |
7bf7f92
to
5f4960d
Compare
@Keavon is it necessary to keep this open as it is accruing many merge conflicts? The code here is not very good anyway. |
I'm happy to keep rebasing it, I think it provides us some value in having the option (when I get time or can assign it to a new contributor) to finish this up since even if it has some code issues it is probably still an upgrade over the current text situation, right? Although if you envision a better approach that's different from this, it might be helpful to know your perspective on that for the context about where this differs from what you'd consider the best route to go with it. |
Closes #1105
Featuring:
A comment from Discord: