-
Notifications
You must be signed in to change notification settings - Fork 178
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
Add text highlight mode for text background #684
Conversation
Size Change: +1.56 kB (0%) Total Size: 509 kB
ℹ️ View Unchanged
|
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.
Looks good. I just need to understand the need for the clone in particular.
* master: (81 commits) Bump webpack from 4.42.0 to 4.42.1 (#740) Lint fixes Move DEFAULT_LINK declaration to within LinkPanel Bump lint-staged from 10.0.8 to 10.0.9 (#728) Fix clip path on iOS Chrome (#727) Bump @wordpress/components from 9.2.4 to 9.2.5 (#722) Performance & aesthetic improvements for drop targets (#687) Fix LinkType default arg hack due to module loading errors Temp fix for LinkType problem lints Bump prettier from 2.0.1 to 2.0.2 (#721) Lots and lots and lots of tests fixed tests transparent color formatting Color migrated Migrated most of the panels, except for color Save on selection change News tests for DesignPanel and related features Auto-submit for all forms Text style support ...
* master: Background element refactor (+ drop targets and fixes) (#693)
@@ -103,10 +123,24 @@ function TextDisplay({ | |||
: ''; | |||
}); | |||
|
|||
if (backgroundType === BACKGROUND_TEXT_MODE.HIGHLIGHT) { | |||
return ( | |||
<HighlightElement ref={ref} {...props}> |
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's really underseriable to change refs. Any way this can be done differently? Some top-level container? Or we can apply the exact same DOM structure of p > span
, but customize the CSS for both p and span
when a specific background mode is used.
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.
Interesting. I will try this top-level container
approach for this case. Thanks, @dvoytenko !
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 did not address this one, though. Is it necessary?
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.
@dvoytenko @barklund what's the resolution here? Dima, can you advise on whether this should still be addressed before merging?
hi Beto, excited to see this taking shape! A couple of issues that I'd love to get addressed:
Feel free to ping me if you have any questions. |
# Conflicts: # includes/Dashboard.php
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
@googlebot I consent. |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
# Conflicts: # assets/src/edit-story/elements/text/display.js # assets/src/edit-story/elements/text/output.js # assets/src/edit-story/utils/textMeasurements.js
I been struggling with this one all afternoon. I simply cannot get the word breaks to align in different calculations. It just can't happen. A scaled text box does not break at the same breakpoints - fonts just don't work like that. In this video here: the left-most box is a visualization of the output render at 30% scale (it's calculated at the full 1080x1920 px size, then scaled down using The right-most version is the rendered story using percentages of the whole story width - directly calculated from the 1080x1920px valued just by ratios instead. As you can see, there will be cases where the calculated height doesn't match the visual height. I can make sure it errs on either side though, so it's always guaranteed that all the lines will fit inside the visual box (but the box might be a line or two too long), or that the box will never be too long (but might cut off the last line or two). And this isn't only an issue for highlight text of course. I can find the same pixel-tight deviations for regular text as well. The only "real" solution as I see it is to manually find line breaks in one of these instances and calculate number of lines from that - and then use explicit line breaks in the real output to match this calculation. We can calculate line breaks by pasting word for word into a fixed-width box and see when the height shifts or we can leverage @pbakaus, what's your call here? Do you have prior experience? Should we manually split the lines to ensure similarity or can we live with the slight shifts? The cut-off text is probably a big no-go here, right? |
@barklund my condolences, this is indeed a tricky thing to do, and I've faced similar issues before. The browser's layout process works in mysterious ways sometimes.. So a couple things:
Here's what I'd do: After resizing etc. (when you're ready to 'commit' a value), measure the total height of the span (fine to use offsetHeight or something here, causes layout but that's the lesser evil in this scenario) to find out how many lines you got, compare it to the total height of the percentage-based/resized 'finalized state' off screen, then, if the line count if off by one in either direction, add a 'padding' to the calculated float (experiment here, ~0.1 or ~0.01% might be enough) to correct. If it helps, I'm happy to jump on a call for a pair programming / troubleshooting session or whatever. Good luck! |
@pbakaus, the trick is though, that I can't test render the final story. At least we don't do that right now. I can only story some variables, that will be used to render the final story, but I can't actually "test run" it and see how long the text will be, how many lines will fit. Also, this won't ever be static anyway, as the rendered story has width, height and font size in percent of the overall page size. Though these will relatively stay the same as the page resizes, the line breaks will change in the rendered story when the page resizes - that's unavoidable. I don't think we can find some magic relative width and height, that'll make all texts break correctly for all page sizes. As far as I can see, we can only do two things to "fix" this:
|
@barklund I'd be very surprised if 1) doesn't produce an accessibility / screen reader nightmare.. so I'm definitely leaning towards 2. But I'm not convinced that we can't simulate the final output under the hood in the editor. What stops us from recreating the conditions in the final output? |
I actually just want to insert
The output is not just one size. The output can be rendered in any size, and we'd have to check that the line breaks are correct for all of them - or at least a representative number of them. That sounds expensive! |
@pbakaus Hm, it seems the rendered story will always break lines at the same points at various viewport sizes - or at least I can't seem to find any combination of text size, font and width that'll make any line break differently at different viewport sizes. So that's a huge first step at least. But in order to test how a rendered story will render a given text field, we need to create an iframe with an AMP story inside and measure it here? Or can we create "normal" html elements and assume they'd render identically? I'm not completely aware how much "magic" (if any) that AMP adds to the browser rendering engine. |
@barklund I don't think there's a crazy amount of magic, it depends more on the way resizing is done (transforms vs. pecentage vs. font-size vs. zoom). At this point, I'd say edge cases are OK, I'm mostly looking for a good enough approximation. Let's merge this soon and then polish later? @dvoytenko I'd say ok to go ahead and review again. |
@@ -41,7 +41,7 @@ const Element = styled.p` | |||
${elementWithFont} | |||
${elementWithTextParagraphStyle} | |||
|
|||
opacity: 0; | |||
opacity: 0; |
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.
Are there any special highlight work in frame? We use an invisible text to figure out which part of an element is selected for editor.
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.
Not completely sure what you mean here. Currently there's no special handling in the frame for a highlight text element. What should happen in the frame, if things were done correctly?
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'd be ok to move it to a separate pull request, as long as we don't forget about this entirely. But this is actually originally your code. We generally try to make sure the text in the frame is displayed (but not visible) the same way as display component to ensure that selection logic is correct. See this: https://github.com/google/web-stories-wp/blob/c6af16cfc77e8fc02efe215358a20e8dc8fd3f64/assets/src/edit-story/elements/text/frame.js#L111-L114
assets/src/edit-story/migration/migrations/v0010_setBackgroundTextMode.js
Outdated
Show resolved
Hide resolved
…-wp into add/text-highlighting
I feel this is working pretty well now. There are many cases where line breaks occur differently in the editor preview vs the rendered story, but that's very hard to do anything about IMO. I've made sure that the text field is always at least as big as the contents, but sometimes it's too big (the test render has an extra line, thus room for an extra line in the preview), but that's erring on the right side, I feel. I'll fix the edit component for highlight now. I don't really understand the purpose of the frame component, so not sure what needs fixing for this one. @dvoytenko, PTAL again. |
I simply cannot get highlight stying to work inside the editor component (draft.js). I don't really know what to do here. I have now made sure the edit component uses more or less the same line height as the display one, but there's still a lot of visual differences between editing and the resulting look. It's not a good user experience. but I recommend we merge this as-is and follow up asap. Maybe someone else has better ideas for how to solve these last remaining issues? |
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 ok with this merging. IMHO let's look into edit mode and frame in separate pull requests. Please file the bugs though. Also, v0010 migration should now be bumped +1.
Bump migration version once again!
Resolves #468