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

Author: Show only users capable of authoring posts #2525

Merged
merged 4 commits into from
Aug 28, 2017

Conversation

aduth
Copy link
Member

@aduth aduth commented Aug 24, 2017

Related: #2029
Closes #2491
Closes #2157
Fixes #2524

This pull request seeks to account for user capabilities in deciding which options to show in the Author select dropdown. It also refactors the component to use the withAPIData higher-order component introduced in #1974, fixes an issue with author selection permanently causing a post to be flagged as dirty, and adds unit tests.

Testing instructions:

Repeat testing instructions from #2100

@aduth aduth added the General Interface Parts of the UI which don't fall neatly under other labels. label Aug 24, 2017
@codecov
Copy link

codecov bot commented Aug 24, 2017

Codecov Report

Merging #2525 into master will increase coverage by 0.36%.
The diff coverage is 92.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2525      +/-   ##
==========================================
+ Coverage    30.2%   30.57%   +0.36%     
==========================================
  Files         174      174              
  Lines        5270     5263       -7     
  Branches      900      897       -3     
==========================================
+ Hits         1592     1609      +17     
+ Misses       3121     3101      -20     
+ Partials      557      553       -4
Impacted Files Coverage Δ
editor/sidebar/post-author/index.js 85% <92.3%> (+85%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f0fe6f...e0f11fb. Read the comment docs.

},
},
)( withInstanceId( PostAuthor ) );
} ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we should enforce a convention here, something like this;

const connectComponent = connect( ... );
const fetchAPIData = withApiData( ... );

export default flowRight( [
  connectComponent,
  fetchAPIData,
] )( Component );

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe. Naming things is hard 😄 For example, the API data higher-order component doesn't necessarily incur a fetch (might be used only for updating), and itself doesn't perform the fetch. All it does is apply a wrapping (higher-order) component.

I think if we were to, we'd want some easy convention for naming, like a consistent prefix. Maybe "apply": applyConnect, applyWithAPIData

Copy link
Contributor

Choose a reason for hiding this comment

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

sound good to me. I'll follow the convention chosen here in my PRs

Copy link
Member Author

Choose a reason for hiding this comment

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

Out of curiosity, what's your reason for wanting a convention here? Are you finding this could be difficult to interpret?

Copy link
Contributor

Choose a reason for hiding this comment

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

My primary reason is to avoid indentation changes when moving from one HoC to composed HOCs. But I also find it more readable.

@aduth aduth force-pushed the update/user-author-permissions branch from 6c82952 to e0f11fb Compare August 27, 2017 23:44
@aduth aduth merged commit cbb0a4d into master Aug 28, 2017
@aduth aduth deleted the update/user-author-permissions branch August 28, 2017 00:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
General Interface Parts of the UI which don't fall neatly under other labels.
Projects
None yet
2 participants