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 Delivery section on Routes page #791

Merged
merged 6 commits into from
Oct 24, 2022

Conversation

RabeaGleissner
Copy link
Contributor

Started working on this issue: #777

Created two components which can be reused across the different sections on the Routes page

  • IconWithText
  • PhotoCredits
    Created a component for the Delivery section

Will continue the refactoring next week!

Screenshot of new (on the left) and current (on the right)
image

@ghost
Copy link

ghost commented Oct 20, 2022

👇 Click on the image for a new way to code review
  • Make big changes easier — review code in small groups of related files

  • Know where to start — see the whole change at a glance

  • Take a code tour — explore the change with an interactive tour

  • Make comments and review — all fully sync’ed with github

    Try it now!

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map Legend

Copy link
Contributor

@jtfairbank jtfairbank left a comment

Choose a reason for hiding this comment

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

Using conventional comments for the review.

  • suggestion(non-blocking): The IconWithText component seems fairly useful / reusable. I'd move it to an icon component folder so it's more visible to folks working on other pages. Note that the style of the photo credit is unique to these pages and we're handling it more generally on another PR, so no need to move the PhotoCredit component.

  • suggestion(non-blocking): We have some end-to-end tests which cover routes, in /e2e-tests/home.spec.ts. But as we refactor the routes page it'd be good to write a general e2e test for routes which ensures something simple, like the title of each section is shown. I don't think there's anything complex enough here to need unit testing.

@RabeaGleissner
Copy link
Contributor Author

Thanks @jtfairbank ! Both good points. I moved the IconWithText to its own directory. And I'll write the E2E test in a separate PR!

@RabeaGleissner RabeaGleissner merged commit 8201d5c into saga Oct 24, 2022
@RabeaGleissner RabeaGleissner deleted the routes_refactor_delivery_component branch October 24, 2022 07:30
@jtfairbank jtfairbank added the hacktoberfest-accepted Hacktober Fest PR Accepted label Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Hacktober Fest PR Accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants