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

add fullscreen to code blocks #10784

Merged

Conversation

Winner95
Copy link
Contributor

@Winner95 Winner95 commented Oct 12, 2020

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

I added fullscreen functionality for code blocks via panel, shown on hover. There are some updates for highlight code block, which allow division of concerns in code:

  • classnames starting with js- for event-listeners - in order to be independent from markup.
  • added 'highlight__panel-action', which allow to add several elements (action-icons, e.g. copy) by the same pattern, which will result in proper styling
  • added store of window scroll position and preventing scroll on document on fullscreen mode
  • added event-listener for escape key on keyboard for switching off the full-screen
  • scss is written in BEM, using some utility classes.
  • in order to keep size of markdown_parser_spec.rb below 250 lines, divide it into two files.
  • added padding for scrolled code on right side

This is how highlight block was before:
before

This is after:

after2

after

after3

Related Tickets & Documents

Closes #8990

QA Instructions, Screenshots, Recordings

  • add different code blocks to the page
  • hover over code blocks to see the fullscreen button
  • click on button to enter fullscreen mode
  • click on button back / press 'escape' button

Added tests?

  • yes
  • no, because they aren't needed
  • no, because I need help

Added to documentation?

  • docs.forem.com
  • readme
  • no documentation needed

PS: I hope this pull-request will be helpful:)

@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Oct 12, 2020
Copy link
Contributor

@citizen428 citizen428 left a comment

Choose a reason for hiding this comment

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

Tried it out locally, works well! Thanks for the contribution 😃

@pr-triage pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 12, 2020
Copy link
Contributor

@ludwiczakpawel ludwiczakpawel left a comment

Choose a reason for hiding this comment

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

Oh, this works great, lovely job!

I have few tiny requests but nothing really major - left comments in the code..
Also, is there a chance to simplify that markup? It feels a bit overkill for one button:

image

app/assets/stylesheets/components/syntax.scss Outdated Show resolved Hide resolved
app/assets/stylesheets/components/syntax.scss Outdated Show resolved Hide resolved
app/assets/stylesheets/components/syntax.scss Outdated Show resolved Hide resolved
@pr-triage pr-triage bot added PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Oct 12, 2020
@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes labels Oct 12, 2020
@Winner95
Copy link
Contributor Author

@ludwiczakpawel thank you for your feedback, updated PR, reduced markup and kept possibility to add any new buttons in action-panel

Copy link
Contributor

@ludwiczakpawel ludwiczakpawel left a comment

Choose a reason for hiding this comment

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

One issue in the comments, though not a blocker.

But also, I've just realized that old posts do not have an icon in the code blocks:

image

I guess it's because markup is not generated on the fly but stored somehow? I don't know tbh...

I don't think it matters that much for "older posts" so if there's not an easy way to fix it, I'd at least try to hide it for older posts and make this functionality "available" for new posts only.

Comment on lines 41 to 43
.highlight {
overflow: auto;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There's one last thing we could try to fix... So this style changes a little bit the behavior of how overflow looks like.. On prod, when you have long lines in highlight the code is actually from edge to edge when scrolling.

Because of that additional styling here ^ there's an offset on the left and right side, defined by the padding, so the code is no longer from edge to edge.

I do understand why you have this styling here (so the buttons are sticked to the corner of the code block), although maybe we could try to bring the previous styling back (so code will be edge to edge when scrolling). I think the trick here is to move the padding from the outer .highlight to the inner one. But this may need a bit more testing though.

If you need help with it, I'm happy to jump in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ludwiczakpawel, sure, I will check it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I move padding to inner element - .highlight, where overflow is and it worked. Now we also have proper calculation of paddings (screenshot will be attached) for wide scrollable area (before text had no right padding at the end of the line).

@pr-triage pr-triage bot added PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 13, 2020
@rhymes
Copy link
Contributor

rhymes commented Oct 13, 2020

@ludwiczakpawel

I guess it's because markup is not generated on the fly but stored somehow? I don't know tbh...

Yes, on save an article turns the markdown into HTML, that's stored in the DB and in the cache at the first request. Everytime an article is updated the cache is wiped and the HTML version in the DB is overwritten.

If we need to update all old articles we need an update script to resave all existing articles to update their markup. It's a possibility.

Copy link
Contributor

@benhalpern benhalpern left a comment

Choose a reason for hiding this comment

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

This looks great to me.

@ludwiczakpawel @rhymes Because it has the if ... action built in, this is resilient to the old posts not immediately being compatible....

We'll handle this scenario as a one-off, but yeah we should seek to create a more general framework around backfilling breaking changes. There's no easy answer, but we're definitely outgrowing the old fragile way quickly.

@Winner95 only thing left to do is make sure all the tests are updated. It looks like the only failing tests are approval tests. See info here: https://docs.forem.com/tests/approvals-tests/

I approve this pending the tests passing.

@nickytonline
Copy link
Contributor

Thanks so much for the PR @Winner95. Any chance you can put some screenshots in the PR description showing the functionality in action. Thanks!

@Winner95 Winner95 requested a review from a team as a code owner October 13, 2020 20:19
@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes labels Oct 13, 2020
Copy link
Contributor

@nickytonline nickytonline left a comment

Choose a reason for hiding this comment

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

Thanks for making the change @Winner95! 🚀

@pr-triage pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 13, 2020
@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Oct 13, 2020
Copy link
Contributor

@ludwiczakpawel ludwiczakpawel left a comment

Choose a reason for hiding this comment

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

🎉🎉🎉

@pr-triage pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 14, 2020
@ludwiczakpawel ludwiczakpawel merged commit 4f79da2 into forem:master Oct 14, 2020
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Oct 14, 2020
boardfish pushed a commit to boardfish/forem that referenced this pull request Oct 17, 2020
* add fullscreen to code blocks

* add review fixes

* remove extra import

* fix approved tests

* code review - paddings

* fix padding to make highlight code scrollable
within  padded area

* add fullscreen to existing webpack entrypoint

* fixed aria: true generated extra ids

* fix tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Lightbox mode" for code snippets
6 participants