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

Refactor GalleryImage to use React hooks #25305

Closed
wants to merge 6 commits into from

Conversation

ravichdev
Copy link

@ravichdev ravichdev commented Sep 14, 2020

Description

Related to #22890

Refactor GalleryImage component to use React hooks

How has this been tested?

  1. Edit page or post.
  2. Add gallery block.
  3. Add/edit/remove images to the block from media library and by manual upload.

Types of changes

Refactor class component to functional component and use React hooks

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.

@ZebulanStanphill ZebulanStanphill added [Block] Gallery Affects the Gallery Block - used to display groups of images [Package] Block library /packages/block-library [Type] Code Quality Issues or PRs that relate to code quality labels Sep 14, 2020
@ravichdev
Copy link
Author

@ZebulanStanphill I pushed up a commit which updates the gallery-image.native.js file to use React hooks. Can you review again ?

onSelectImage() {
if ( ! this.props.isSelected ) {
this.props.onSelect();
const onSelectCaption = () => {
Copy link
Member

Choose a reason for hiding this comment

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

Personally, I prefer named functions, because they're easier to debug. I don't think there's a good reason to assign anonymous functions to a constant here.

Copy link
Member

@ellatrix ellatrix left a comment

Choose a reason for hiding this comment

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

Looks good. Would be nice to update with named functions. Maybe needs an extra green light from @WordPress/native-mobile?

@glendaviesnz
Copy link
Contributor

glendaviesnz commented Nov 9, 2020

just a note that this is going to be in conflict with some of the work in #25940, and also with some native code refactoring that is being done in relation to that ... obviously we can just pick through the changes when we are able to rebase again if this change is urgent and needs to go in before we finish the gallery refactor, but just highlighting in case this change can be deferred until gallery changes are finalised.

@dratwas do you have any thoughts on whether we try and get this rebased into the gallery PR before you branch off again to to finish the native work there, or not?

@dratwas
Copy link
Contributor

dratwas commented Nov 10, 2020

@dratwas do you have any thoughts on whether we try and get this rebased into the gallery PR before you branch off again to to finish the native work there, or not?

Hey @glendaviesnz, thanks for asking :) AFAIK we will remove these files during the gallery refactor work so there is no difference for me :) Resolving conflicts should not be a problem in that case.

@glendaviesnz
Copy link
Contributor

glendaviesnz commented Nov 10, 2020

AFAIK we will remove these files during the gallery refactor work

🤦 Doh! must have been a bad day yesterday, even after looking at the file I had 'Image' component in my mind instead of 'GalleryImage' ... @ravichdev & @ellatrix completely ignore my irrelevant comment! ... but just be aware that this component will be deleted if we manage to get the Gallery refactor merged.

@youknowriad
Copy link
Contributor

Hey there! can we rebase and land this?

@paaljoachim paaljoachim mentioned this pull request Feb 21, 2021
26 tasks
Base automatically changed from master to trunk March 1, 2021 15:44
@talldan
Copy link
Contributor

talldan commented Jul 5, 2021

@ravichdev I think this needs a rebase and some conflicts need to be resolved. Is that something you'd be able to look into?

@glendaviesnz
Copy link
Contributor

glendaviesnz commented Jul 5, 2021

@ravichdev I think this needs a rebase and some conflicts need to be resolved. Is that something you'd be able to look into?

It maybe isn't worth landing this now as gallery refactor is close to being merged as the mobile work is complete, and the gallery image component is being deprecated. The old gallery image component code will live in a deprecated subfolder for a short period, but unlikely to get any further development.

@glendaviesnz
Copy link
Contributor

Going to close this. Now that the gallery refactor is merged the old version of the gallery code is being deprecated, so probably not worth refactoring at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Gallery Affects the Gallery Block - used to display groups of images [Package] Block library /packages/block-library [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants