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

Gallery Refactor #1791

Merged
merged 5 commits into from
Sep 27, 2019
Merged

Gallery Refactor #1791

merged 5 commits into from
Sep 27, 2019

Conversation

sirugh
Copy link
Contributor

@sirugh sirugh commented Sep 26, 2019

I was going to create a talon for the Gallery component and I started doing a little refactoring. Turns out we can pretty much remove the GalleryItems component and we can simplify how Gallery works. In doing this I discovered that there's pretty much no talon to be written for Gallery.

It is important to note that Gallery is used in Category and in Search. In both cases there may be a different number of incoming or expected items being rendered which was counterintuitive when you looked at the hardcoded pageSize value.

So what I did was remove the pageSize prop and instead put the onus on the parent component. In this case, Category knows that it expects 6 items so it generates an empty placeholder array.

@sirugh sirugh added the version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump. label Sep 26, 2019
@sirugh sirugh requested a review from jimbo September 26, 2019 16:17
@sirugh sirugh changed the title Rugh/talon gallery Gallery Refactor Sep 26, 2019
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Sep 26, 2019

Fails
🚫 Missing "Description" section. Please add it back, with detail.
🚫 Missing "Verification Steps" section. Please add it back, with detail.
🚫

No linked issue found. Please link a relevant open issue by adding the text "closes #<issue_number>" or including the associated JIRA ID in your PR.

Messages
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).

If your PR is missing information, check against the original template here. At a minimum you must have the section headers from the template and provide some information in each section.

Generated by 🚫 dangerJS against e52aaec

Copy link
Contributor

@jimbo jimbo left a comment

Choose a reason for hiding this comment

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

This is fine. Overall, I don't think there's a lot of value in eliminating GalleryItems, but it's good to strip out the hardcoded pageSize.

@sirugh
Copy link
Contributor Author

sirugh commented Sep 27, 2019

I disagree -- GalleryItems wasn't doing anything important and I'd rather simplify the codebase than keep old things around just because.

@dpatil-magento dpatil-magento merged commit 103cc25 into develop Sep 27, 2019
@dpatil-magento dpatil-magento deleted the rugh/talon-gallery branch September 27, 2019 18:00
@jimbo jimbo mentioned this pull request Oct 2, 2019
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:venia-ui version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants