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

Integration of wordpress blog #1

Merged
merged 17 commits into from
Dec 27, 2019
Merged

Conversation

mailtime-wing
Copy link
Collaborator

Please review. Thanks!

@mailtime-wing mailtime-wing added the enhancement New feature or request label Dec 18, 2019
Copy link
Collaborator

@chochihim chochihim left a comment

Choose a reason for hiding this comment

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

Please take a look on all of my comments on specific file/lines. Also, please fix below in general

  1. Please use .svg instead of .png
  2. You don't need to always wrap your component in <>...</>
  3. Is it possible to use your Button component to replace usage of the purple-arrow image? Also please do not use such a generic name for this component.
  4. Please use <FormattedMessage /> for static text. Add corresponding translation to src/intl as well.
  5. Sub-components should be put above main component

src/theme.js Outdated Show resolved Hide resolved
gatsby-node.js Outdated Show resolved Hide resolved
src/templates/post.js Outdated Show resolved Hide resolved
src/pages/press.js Outdated Show resolved Hide resolved
src/pages/index.js Outdated Show resolved Hide resolved
src/components/Tag/index.js Outdated Show resolved Hide resolved
src/components/DataInsights/index.js Outdated Show resolved Hide resolved
src/components/Button/index.js Outdated Show resolved Hide resolved
src/components/BlogPost/index.js Outdated Show resolved Hide resolved
<ReadMore>Read more ...</ReadMore>
<ViewCountContainer>
<ViewCountImage src={viewCountIcon} />
<ViewCount>320</ViewCount>
Copy link
Collaborator

Choose a reason for hiding this comment

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

How can we get this info instead of hardcoded?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hided view count right now.

Don has created the view counts in wordpress, but cannot query in graphql(do not have that field in allWordpressPost/wordpressPost).
The only way to get the view count is getting from below url with postId
https://blog.measurable.ai/wp-json/pvc/v1/view/ + post id
Any suggestions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets handle this later

@chochihim chochihim removed the request for review from kitgary December 19, 2019 09:42
Copy link
Collaborator

@chochihim chochihim left a comment

Choose a reason for hiding this comment

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

Please check my comments and also:

  1. Please check again all redundant usages of <>...</>
  2. Avoid creating redundant components

src/components/BlogPost/index.js Outdated Show resolved Hide resolved
src/components/BlogPost/index.js Outdated Show resolved Hide resolved
src/components/Tag/style.js Outdated Show resolved Hide resolved
src/pages/index.js Outdated Show resolved Hide resolved
src/components/Tag/index.js Outdated Show resolved Hide resolved
src/components/DataInsights/index.js Outdated Show resolved Hide resolved
src/components/DataInsights/index.js Outdated Show resolved Hide resolved
@gemscng gemscng requested a review from kitgary December 20, 2019 06:19
Copy link
Collaborator

@chochihim chochihim left a comment

Choose a reason for hiding this comment

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

One comment left. Please also fix style as discussed.

src/components/DataInsights/index.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@chochihim chochihim left a comment

Choose a reason for hiding this comment

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

Please match style with design on Zeplin. Also prefer to use margin instead of top to control distance between elements

@chochihim chochihim merged commit 7646b6e into measurableai:master Dec 27, 2019
@Jellygoo Jellygoo mentioned this pull request Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants