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

[RNMobile] Add badges on gifs #19111

Closed
wants to merge 1 commit into from

Conversation

kmdupr33
Copy link

@kmdupr33 kmdupr33 commented Dec 12, 2019

Description

This pr adds badges to the GIF images.

This feature was implemented to support some reusability of badges (since the issue mentions that badges are something we're interested in reusing). The Badge and Badgeable components in badgeable.native.js are how we achieve this (limited) reusability.

Badge, as the name suggests, is a simple presentational component that merely handles the styling of the badge and takes in a text prop to support arbitrary text inside the badge.

Badgeable can wrap a component we want to display a badge overtop it's content. A Badgeable component allows us to easily add badges to existing components without duplicating code for overlaying and positioning the Badge component. Moreover, with this approach, changing the positioning of badges can be done in one place instead of every place where a badge is visible.

HOCs and render props were briefly considered as a way of accomplishing this same sort of DRY implementation, but ultimately I decided that these two solutions were overkill. Although either of these solutions could have worked, Badgeable doesn't need add or share state or functionality with the component it's wrapping, so a simpler wrapper component is a better solution.

How has this been tested?

The main sample post has a GIF image, so I simply opened the app and checked to see if the GIF badge was visible over the GIF image and not visible over other images that weren't GIFs.

Screenshots

Screen Shot 2019-12-12 at 6 47 15 PM

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

Fixes wordpress-mobile/gutenberg-mobile#968

Copy link

@test-case-reminder test-case-reminder bot left a comment

Choose a reason for hiding this comment

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

Here are some suggested test cases for this PR.

WordPress iOS:

  • Simultaneous uploads - steps
  • Image block - Insert image from device (failing) - steps
  • Image block - Insert image from device (cancel) - steps
  • Image block - Add Caption - steps
  • Image block - Close/Re-open post with an ongoing image upload - steps
  • Image block - Close post with an ongoing image upload - steps
  • Image block - Switch to classic editor with an image block in page - steps

WordPress Android:

  • Simultaneous uploads - steps
  • Image block - Insert image from device (failing) - steps
  • Image block - Insert image from device (cancel) - steps
  • Image block - Add Caption - steps
  • Image block - Close/Re-open post with an ongoing image upload - steps
  • Image block - Close post with an ongoing image upload - steps
  • Image block - Switch to classic editor with an image block in page - steps

If you think that suggestions should be improved please edit the configuration file here. You can also modify/add test-suites to be used in the configuration.

If you are a beginner in mobile platforms follow build instructions.

@Tug Tug added the [Status] In Progress Tracking issues with work in progress label Dec 16, 2019
@Tug
Copy link
Contributor

Tug commented Dec 16, 2019

@kmdupr33 A quick feedback on the format of your PR.

I think you could have created a draft PR instead of having it as part of the title https://github.blog/2019-02-14-introducing-draft-pull-requests/
But it's ok, I added the "[Status] In Progress" label which has similar meaning ;)

first draft of adding gif functionality #19111

Could you update the title to be more specific? Gutenberg (native) already supports gifs, what we're adding here is the badge. Another nice to have is to use a prefix to indicate that it's a gutenberg mobile related PR: [RNMobile ] PR title.

This pr resolves an issue for gutenberg-mobile

We usually adds that at the end of the PR with the format:

Fixes wordpress-mobile/gutenberg-mobile#968

Github is going to prettify the url as much as possible and it's clearer to see the actual issue number before hovering it to get more info.

Copy link
Contributor

@Tug Tug left a comment

Choose a reason for hiding this comment

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

I had a few comments but the code looks good so far 👍

As discussed on slack I believe Badge/Badgeable could be moved to packages/components/mobile so we can reuse them for other media content in the future

packages/block-library/src/image/styles.native.scss Outdated Show resolved Hide resolved
packages/block-library/src/image/styles.native.scss Outdated Show resolved Hide resolved
packages/block-library/src/image/styles.native.scss Outdated Show resolved Hide resolved
packages/block-library/src/image/badgeable.native.js Outdated Show resolved Hide resolved
packages/block-library/src/image/badgeable.native.js Outdated Show resolved Hide resolved
@kmdupr33 kmdupr33 changed the title first draft of adding gif functionality [RNMobile] Add badges on gifs Dec 16, 2019
@kmdupr33 kmdupr33 force-pushed the issue-968-add-gif-badge branch 2 times, most recently from 5848bbb to e5e9d86 Compare December 16, 2019 12:13
@Tug
Copy link
Contributor

Tug commented Dec 17, 2019

Quick update for @iamthomasbishop : This is how the badge looks like now (on Android)

image

@kmdupr33 kmdupr33 force-pushed the issue-968-add-gif-badge branch from e5e9d86 to ca33b4f Compare December 17, 2019 18:05
@iamthomasbishop
Copy link

The styling looks about right, other than the font-size diff that was already mentioned. Thanks for the update!

@kmdupr33 kmdupr33 force-pushed the issue-968-add-gif-badge branch 5 times, most recently from 1755b06 to 7e91e53 Compare December 18, 2019 15:28
Copy link
Contributor

@Tug Tug left a comment

Choose a reason for hiding this comment

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

This LGTM 👍
(You'll need to rebase and fix conflicts before we can merge this PR)

@kmdupr33 kmdupr33 force-pushed the issue-968-add-gif-badge branch 2 times, most recently from 7e91e53 to 7a69ded Compare December 21, 2019 20:16
@kmdupr33 kmdupr33 force-pushed the issue-968-add-gif-badge branch from 7a69ded to 7b95b89 Compare January 4, 2020 03:22
@kmdupr33 kmdupr33 force-pushed the issue-968-add-gif-badge branch from 7b95b89 to afa2f84 Compare January 4, 2020 14:23
@gziolo gziolo added [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) labels Nov 28, 2020
@gziolo gziolo removed the [Status] In Progress Tracking issues with work in progress label Nov 28, 2020
@gziolo
Copy link
Member

gziolo commented Nov 28, 2020

Any plans to rebase and merge this PR or should it be closed? It looks abandoned 🙁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add GIF badge to GIFs in editor
4 participants