Skip to content

Conversation

@kenny-not-dead
Copy link
Contributor

close #xxx

I continued implementing text underlining, considering the possibility of installing different options for underlining, it seemed logical to me to have a drop-down list of possible options

If this implementation is suitable, I can add this in sheets

I hope you like this implementation, I personally like it =)

open demo -> try use underline

Screencast.from.2025-09-07.21-07-33.webm

Pull Request Checklist

  • Related tickets or issues have been linked in the PR description (or missing issue).
  • Naming convention is followed (do please check it especially when you created new plugins, commands and resources).
  • Unit tests have been added for the changes (if applicable).
  • Breaking changes have been documented (or no breaking changes introduced in this PR).

@github-actions
Copy link

github-actions bot commented Sep 7, 2025

View Deployment

📑 Demo (React@19) Demo (React@16) 📚 Storybook
🔗 Preview link 🔗 Preview link 🔗 Preview link

@codecov
Copy link

codecov bot commented Sep 7, 2025

Codecov Report

❌ Patch coverage is 16.00000% with 42 lines in your changes missing coverage. Please review.
✅ Project coverage is 33.28%. Comparing base (6b7bef5) to head (e2b94c1).
⚠️ Report is 5 commits behind head on dev.

Files with missing lines Patch % Lines
packages/docs-ui/src/controllers/menu/menu.ts 0.00% 33 Missing ⚠️
...ocs-ui/src/components/underiline-picker/picker.tsx 25.00% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #5818      +/-   ##
==========================================
- Coverage   33.30%   33.28%   -0.02%     
==========================================
  Files        2761     2763       +2     
  Lines      144913   144959      +46     
  Branches    32345    32355      +10     
==========================================
- Hits        48265    48254      -11     
- Misses      96648    96705      +57     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kenny-not-dead
Copy link
Contributor Author

@jikkai
please clarify can i leave a TODO or should i delete it? i will probably add it myself, but a bit later. i would also like to clarify about the message from the bot about test coverage, i didn't see any tests on the menu here, similar to sheets

@jikkai
Copy link
Member

jikkai commented Sep 8, 2025

Apologies for the late reply. I’ve been handling a few other priorities 🤧
Thank you, as always, for your continued support 🫶

From a review perspective your implementation looks perfectly fine. However, as it may relate to Univer Docs import/export, we’ll need a little extra time to review the implications :)

@kenny-not-dead
Copy link
Contributor Author

@jikkai I understand you, I will wait for an answer on this issue =)

Considering that I also did import and export on my backend, I think it will not affect you for long, since your underscore types were initially well designed. I think the only issue is in supporting the necessary switch-case types

@kenny-not-dead
Copy link
Contributor Author

kenny-not-dead commented Sep 18, 2025

@jikkai

I decided to expand on this PR. First, I decided to make it more similar to other dropdowns, as I hadn't previously realized that the hover value applies to the entire element. I also expanded dark theme support and added two type variants. Overall, I decided to make this component more similar to a table component.

image image

@kenny-not-dead
Copy link
Contributor Author

It's weird, this test used to work fine, but I don't think I added anything that would have broken it.

@jikkai jikkai requested a review from VicKun4937 September 19, 2025 03:43
@jikkai
Copy link
Member

jikkai commented Sep 19, 2025

It's weird, this test used to work fine, but I don't think I added anything that would have broken it.

This is an issue caused by pixel comparison. As long as the content in the examples of docs or sheets is modified, resulting in changes to the rendering of the visible area, this problem will be triggered.
This usually does not constitute a real issue, we will fix it after merging :)

@kenny-not-dead
Copy link
Contributor Author

@jikkai

Thanks, then I'll wait for confirmation of this decision. I think it's gotten better after the last change. I also have a suggestion regarding underscores in tables, since Excel also supports double underscores.

@kenny-not-dead
Copy link
Contributor Author

@VicKun4937
Please clarify if this PR is still relevant?

@kenny-not-dead
Copy link
Contributor Author

I think this can be closed...

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.

2 participants