-
Notifications
You must be signed in to change notification settings - Fork 110
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
#986 fix space takeup for url link icon #2168
Conversation
@raineorshine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does fix the case in #986, but breaks the url icon on multiline urls:
(FYI This is covered by this snapshot test, but it is missing on ubuntu
, so you'll need to generate the base snapshot by running the test with -u
on main
.)
Current Behavior
Expected Behavior
main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm now seeing that this does not result in the correct height unfortunately. (Careful with a quick fix of a px offset here; the height needs to work across all font sizes. Please justify your solution.)
Current Behavior
Height of single-line and ellipsized urls are too large.
Expected Behavior
main
I found fixes and pushed the update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found the root cause of this issue - it's due to the a tag modifying the last line of the thought annotation text when it's multiline. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The url link has to be part of the document flow and cause the thought to wrap, otherwise it can get cropped by the right edge of the screen on mobile. There is not enough margin-right on mobile when the thought goes to the end of the line.
(You can enable the mobile layout by using the Device Toolbar in Chrome Dev Tools.)
Maybe try limiting the height of the url link instead?
Hi @raineorshine position absolute on the thought-annotation a tag causes the mobile issue you mentioned, instead I set the a tag height to make sure it doesn't affect the thought annotation text's height. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The url link shifts position when the cursor is set on it:
Screen.Recording.2024-07-26.at.5.38.56.PM.mov
I've split up the url snapshot tests into three tests:
That should cover all the test cases we have discussed in this thread except the mobile test case (normally that would be tested with WebdriverIO but that suite is broken currently). I omitted the failing snapshots until they can be generated correctly. Run |
I've fixed the url icon jumping issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've split up the url snapshot tests into three tests:
- single-line url (failing)
- multi-line url (failing)
- url child (passing)
That should cover all the test cases we have discussed in this thread except the mobile test case (normally that would be tested with WebdriverIO but that suite is broken currently).
I omitted the failing snapshots until they can be generated correctly. Run
yarn test:puppeteer -- snapshot -t url
to run the tests and-u
to update.
I think my commit may have gotten overwritten by your last force push. Sorry that I wasn't clear I had pushed it to the PR branch. I pushed it again (6b100c2).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can confirm the url jumping issue is fixed. Thanks.
Noticing a couple regressions:
- At lower font sizes, the url link now incorrectly wraps. I added ce69a0f to run the url tests at font size 18 and 14 to cover this case.
- The clickable area of the url link has been diminished. The entire link with a few pixels of padding should be clickable, as in
main
. As of 3881190 it's only clickable along the right edge:
Screen.Recording.2024-08-01.at.11.29.17.AM.mov
ce69a0f
to
23a374e
Compare
80019f7
to
10b9d7c
Compare
url wrapping issue and clickable area fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. I have manually tested and confirmed the following cases are working:
- multiline url: url icon should be inline
- multiline url with cursor: height should be the same
- height of single-line and ellipsized urls should be the same
- url icon should not shift position with cursor
- at lower font sizes, url icon wraps
- [mobile] url icon should not be cropped
This issue is partially fixed:
- extended click area - The top, right, and left sides correctly extend the click area. The bottom does not.
Given the nearly complete coverage of the observed issues, I am satisfied with this PR and think it should be merged. I will open a separate issue for the minor click area issue.
Thanks so much for your persistence! :)
No description provided.