Skip to content

Removes Dummy Features from Ashes #553

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

Merged
merged 7 commits into from
Dec 14, 2016
Merged

Conversation

ghost
Copy link

@ghost ghost commented Dec 9, 2016

Removes:
-Email & Notifications Preferences from Customer Details
-SEO section from Product Details
-Assignees field from Assignees/Watchers (since Assignees/Watchers do the same thing)

@ghost ghost requested review from jmataya and ferrerrajc December 9, 2016 00:19
@ghost ghost added WIP and removed ready to review labels Dec 9, 2016
@ghost ghost added ready to review and removed WIP labels Dec 9, 2016
// <a className="fc-watchers__link" onClick={e => this.watch(e, groups.assignees)}>take it</a>
// </div>
// </div>
// {renderGroup(props, groups.assignees)}
Copy link
Contributor

Choose a reason for hiding this comment

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

^ Should we just ehm... remove this lines?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with Pavel's comment.

Copy link
Author

Choose a reason for hiding this comment

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

My thought here was that the feature actually works. Just right now there's no difference between "Assignees" and "Watchers". We need to think through this story and add it back once we figure out what to do with Assignees. @jmataya Would you still prefer me to remove it at this point?

"metaTitle",
"metaDescription"
]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should definitely remove URL, but should we just implement meta title and meta description in Firebrand, TPG, and TD?

Copy link
Author

Choose a reason for hiding this comment

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

@jmataya Right now this isn't implemented anywhere. From the API documentation I don't think it's in the API either for a product detail which is why I removed it. Am I wrong? If it's at least in the API, then I'll add it back in, but right now this section does nothing

Copy link
Contributor

Choose a reason for hiding this comment

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

URL is renamed into Slug in #505
I agree: probably, we should finish SEO section integration in storefronts instead of deleting it completely from ashes.

Copy link
Author

Choose a reason for hiding this comment

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

@Axblade @jmataya I'd rather remove this now - and open a work item to integrate this and add it back in. Right now I would put that feature lower on our priority list than some other items we have to do. In the mean time we are getting customer questions about what this does....

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with that. Let's prioritize making Ashes usable.

// <a className="fc-watchers__link" onClick={e => this.watch(e, groups.assignees)}>take it</a>
// </div>
// </div>
// {renderGroup(props, groups.assignees)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with Pavel's comment.

"metaTitle",
"metaDescription"
]
}
Copy link
Author

Choose a reason for hiding this comment

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

@jmataya Right now this isn't implemented anywhere. From the API documentation I don't think it's in the API either for a product detail which is why I removed it. Am I wrong? If it's at least in the API, then I'll add it back in, but right now this section does nothing

@jmataya
Copy link
Contributor

jmataya commented Dec 13, 2016

Re-running tests to see if we can get them to pass.

Copy link
Contributor

@jmataya jmataya left a comment

Choose a reason for hiding this comment

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

@steven-foxcomm I'm alright with the changes, but can you fix tests before we merge? The tests for watchers are broken.

@ghost ghost dismissed jmataya’s stale review December 13, 2016 20:20

Discussed above - Removed layout.json changes since they are already coming in another PR

@jmataya jmataya merged commit aa10335 into master Dec 14, 2016
@jmataya jmataya deleted the fix/AshesRemoveDummyFields branch December 14, 2016 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants