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.

Sign up for GitHub

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

Jump to bottom

Add wayback-info component to change-view which renders link to Wayback calendar view and page versions #196 #381

Merged
merged 1 commit into from
Jul 29, 2019
Merged

Add wayback-info component to change-view which renders link to Wayback calendar view and page versions #196 #381

merged 1 commit into from
Jul 29, 2019

Conversation

SYU15
Copy link
Contributor

@SYU15 SYU15 commented Jul 14, 2019

Let me know what you think about this especially the terminology for the links and how I wrote the tests. My React is pretty rusty because we use Ember at LinkedIn.

With both version links
Screen Shot 2019-07-14 at 12 28 09 PM

With one version link
Screen Shot 2019-07-14 at 12 27 54 PM

With no version links
Screen Shot 2019-07-14 at 12 24 55 PM

Mr0grog
Mr0grog reviewed Jul 15, 2019
View reviewed changes
Copy link
Member

@Mr0grog Mr0grog left a comment

Choose a reason for hiding this comment

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

Overall, this looks pretty good to me 👍. I added a few comments about some minor things we could improve inline.

src/components/wayback-info.jsx Outdated Show resolved Hide resolved
src/components/wayback-info.jsx Outdated Show resolved Hide resolved
@SYU15
Copy link
Contributor Author

SYU15 commented Jul 19, 2019

I decided to jazz the links up a bit and also rename the component to be more generic if we are expecting at some point to have new diff sources. I am assuming a certain contract with the API right now where the structure of the data will be the same in terms of where I can grab the view_url and the source_type.

Screen Shot 2019-07-18 at 11 17 35 PM

@SYU15 SYU15 marked this pull request as ready for review July 20, 2019 19:03
Mr0grog
Mr0grog reviewed Jul 24, 2019
View reviewed changes
Copy link
Member

@Mr0grog Mr0grog left a comment

Choose a reason for hiding this comment

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

This looks great, and the icons and styling for the links are much nicer. 😄

I added a few minor style/markup nits inline, and one question about the tests that I missed in the earlier review.

src/components/source-info.jsx Outdated Show resolved Hide resolved
src/components/source-info.jsx Outdated Show resolved Hide resolved
src/components/source-info.jsx Outdated Show resolved Hide resolved
src/components/source-info.jsx Outdated Show resolved Hide resolved
src/components/__tests__/source-info.test.jsx Outdated Show resolved Hide resolved
src/components/__tests__/source-info.test.jsx Outdated Show resolved Hide resolved
@SYU15
Copy link
Contributor Author

SYU15 commented Jul 26, 2019

The links still render as expected after refactoring, lmk what you think:
Screen Shot 2019-07-25 at 10 49 15 PM

Mr0grog
Mr0grog approved these changes Jul 26, 2019
View reviewed changes
Copy link
Member

@Mr0grog Mr0grog left a comment

Choose a reason for hiding this comment

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

I dig it. Ready to merge?

@SYU15
Copy link
Contributor Author

SYU15 commented Jul 26, 2019

Yes, thanks! 😃

@Mr0grog Mr0grog merged commit 1a13d57 into edgi-govdata-archiving:master Jul 29, 2019
@Mr0grog Mr0grog mentioned this pull request Aug 1, 2019
Closed
Mr0grog added a commit that referenced this pull request Aug 9, 2019
@Mr0grog
Release #381, #390, #392, and dependency updates
49f79b5
Mr0grog added a commit to edgi-govdata-archiving/web-monitoring-ops that referenced this pull request Aug 9, 2019
@Mr0grog
Deploy UI updates and security fixes
be3ee35 
- edgi-govdata-archiving/web-monitoring-ui#381
- edgi-govdata-archiving/web-monitoring-ui#390
- edgi-govdata-archiving/web-monitoring-ui#392
- UI dependency updates.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mr0grog Mr0grog Mr0grog approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@SYU15 @Mr0grog