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

Implement contact information, addresses, newsletter sections on account details page #570

Closed
wants to merge 12 commits into from

Conversation

Starotitorov
Copy link
Contributor

This PR is a:

[x] New feature
[ ] Enhancement/Optimization
[ ] Refactor
[ ] Bugfix
[ ] Test for existing code
[ ] Documentation

Summary

When this pull request is merged, it will add my account page. Only contact information, addresses and newsletter sections was added #514.

Additional information

@vercel
Copy link

vercel bot commented Nov 30, 2018

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

@Starotitorov Starotitorov force-pushed the feature/implement-account-details branch from d063eaf to 8ac93c1 Compare November 30, 2018 13:39
@coveralls
Copy link

coveralls commented Nov 30, 2018

Coverage Status

Coverage increased (+0.1%) to 57.635% when pulling 202141e on feature/implement-account-details into a11d637 on release/2.0.

@Starotitorov Starotitorov changed the title Feature/implement account details Implement contact information, addresses, newsletter sections on account details page Nov 30, 2018
@Starotitorov Starotitorov force-pushed the feature/implement-account-details branch 3 times, most recently from e10fd4f to 0a4afdf Compare November 30, 2018 14:20
@Starotitorov Starotitorov force-pushed the feature/implement-account-details branch 5 times, most recently from 458aa69 to 93f3e41 Compare December 4, 2018 10:53
@Starotitorov Starotitorov force-pushed the feature/implement-account-details branch from 93f3e41 to 8c57f6a Compare December 4, 2018 10:53
@Starotitorov Starotitorov force-pushed the feature/implement-account-details branch from 8c57f6a to eabf364 Compare December 4, 2018 10:58
@sirugh sirugh self-assigned this Jan 10, 2019
@sirugh sirugh added the hold On hold until another condition is fulfilled. label Jan 10, 2019
@sirugh
Copy link
Contributor

sirugh commented Jan 10, 2019

Hold until #541 is merged.

@sirugh sirugh added enhancement New feature or request pkg:venia-concept and removed hold On hold until another condition is fulfilled. labels Jan 23, 2019
user: {
currentUser: { addresses }
}
}) => addresses.find(({ default_shipping }) => default_shipping);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just a boolean prop? If so this is fine, otherwise it would just return the first default_shipping prop it finds.

}
}) => addresses.find(({ default_billing }) => default_billing);

export const getCurrentUser = ({ user: { currentUser } }) => currentUser;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be replaced by getUserInformation? Seems to be doing almost identical things though getUserInformation returns a subset of the entire currentUser props.

title: PropTypes.string
}),
customer: PropTypes.shape({}),
addresses: PropTypes.arrayOf(PropTypes.shape({}))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should add user to this propTypes object.

title: PropTypes.string
}),
title: PropTypes.node,
rightTitle: PropTypes.node,
Copy link
Contributor

Choose a reason for hiding this comment

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

What is rightTitle? Is this similar to the badge we discussed in the previous PR?

import classify from 'src/classify';
import defaultClasses from './separator.css';

class Separator extends Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this really need to be a class? Can we just hide the last item with css?

import classify from 'src/classify';
import defaultClasses from './actionButton.css';

class ActionButton extends Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need this. We have a button component now.

export const getCountryName = (countries, countryId) => {
const country = countries.find(country => country.id === countryId);

return country ? country.full_name_locale : '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really want to return empty string here? What's the format for this data? Is there a safer fallback we can use or maybe just something like COUNTRY_NAME_UNKNOWN?

@sirugh
Copy link
Contributor

sirugh commented Jan 24, 2019

Closing for now. See #514 for information.

@sirugh sirugh closed this Jan 24, 2019
@supernova-at supernova-at deleted the feature/implement-account-details branch July 25, 2019 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request pkg:venia-concept
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants