Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Refactor header components and add talons #1793

Merged
merged 5 commits into from
Sep 30, 2019
Merged

Conversation

sirugh
Copy link
Contributor

@sirugh sirugh commented Sep 26, 2019

This PR is largely a refactor of header with some moving of code to talons.

  • OnlineIndicator has been moved under the Header/ directory as it didn't make sense as a standalone component.
  • I've added context to some components such as cartTrigger so that they can more easily be used independently of the Header at a later time.
  • I removed cartCounter as it was a with minimal styling.
  • I inlined icons rather than passing them as children of some of the triggers.

@sirugh sirugh requested a review from jimbo September 26, 2019 20:01
@sirugh sirugh added the version: Patch This changeset includes backwards compatible bug fixes. label Sep 26, 2019
@@ -32,13 +32,6 @@
padding: 0 1rem;
}

.navTrigger,
.cartTrigger {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unused classes.

</SearchTrigger>
<CartTrigger {...cartTriggerProps} />
active={searchOpen}
onClick={handleSearchTriggerClick}
Copy link
Contributor Author

@sirugh sirugh Sep 26, 2019

Choose a reason for hiding this comment

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

Renamed props and included SearchIcon directly in the trigger component.

@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 8d92bb8

jimbo
jimbo previously approved these changes Sep 27, 2019
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.

Nicely done all around. 👍

@jimbo jimbo mentioned this pull request Oct 2, 2019
5 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
pkg:peregrine pkg:venia-ui version: Patch This changeset includes backwards compatible bug fixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants