Local/refactor #181
Local/refactor #181
Conversation
- Updates styled components for the story picker module for better alignment.
- Adds class on Web Stories block wrapper if it's an AMP request. - Removed 'glider-js' in favour of npm package for the same.
|
Size Change: -1.81 kB (0%) Total Size: 2.02 MB
ℹ️ View Unchanged
|
| import { ERRORS } from '../../../dashboard/app/textContent'; | ||
|
|
||
| const useStoryApi = (dataAdapter, { editStoryURL, storyApi }) => { | ||
| const [state, dispatch] = useReducer(storyReducer, defaultStoriesState); |
There was a problem hiding this comment.
@swissspidy
As we discussed on PR #150 , in this PR I've taken some part from the web stories dashboard and created separate providers for the block.
Still for the stories data, I'm relying on the existing storyReducer.
There was a problem hiding this comment.
Still for the stories data, I'm relying on the existing
storyReducer.
Because it's so big & complex?
There was a problem hiding this comment.
It does look big and complex at first but got hang of it 😅 . Now for the why we are still using it, we have added feature to rearrange selected stories, which relies on the data return from the existing storyReducer.
Now we can decouple it but at the moment it could take more time to go through the components used and identify how to remove the complete reliance.
This PR has removed reliance on dashboard API all but for the storyReducer. let me know your thoughts.
- Removes chunkSplitting for 'web-stories-scripts' due to issues with the bundle not loading, known issue with webpack v4.
| * Stories 'order-by' values. | ||
| */ | ||
| export const ORDER_BY_OPTIONS = { | ||
| date: { |
There was a problem hiding this comment.
Maybe new-to-old for consistency? Although I do like the simplicity of date...
There was a problem hiding this comment.
I had kept it for simplicity but guess consistency is also important for the maintainability of the source. I'll update it.
| return ( | ||
| <ConfigProvider config={config}> | ||
| {blockType === BLOCK_TYPE_LATEST_STORIES ? ( | ||
| <LatestStoriesEdit | ||
| attributes={attributes} | ||
| setAttributes={setAttributes} | ||
| /> | ||
| ) : blockType === BLOCK_TYPE_SELECTED_STORIES ? ( | ||
| <SelectedStoriesEdit | ||
| icon={icon} | ||
| attributes={attributes} | ||
| setAttributes={setAttributes} | ||
| isSelected={isSelected} | ||
| /> | ||
| ) : ( | ||
| <StoryEmbedEdit | ||
| icon={icon} | ||
| attributes={attributes} | ||
| setAttributes={setAttributes} | ||
| className={className} | ||
| isSelected={isSelected} | ||
| /> | ||
| )} | ||
| </ConfigProvider> | ||
| ); |
There was a problem hiding this comment.
Might be a good opportunity to return early instead of using nested ternaries here, for readability.
if (blockType === BLOCK_TYPE_LATEST_STORIES) {
return ();
}
if ( ...) {
return ();
}
return ();
| /* | ||
| * amp-story-player's 'shadow-root-intermediary' element shows black space on top, | ||
| * because of the 'a' links. Making 'a' absolutely positioned removes them from normal | ||
| * relative flow, removes the space. | ||
| */ |
There was a problem hiding this comment.
Interesting! Maybe something to report upstream?
There was a problem hiding this comment.
though this issue comes on non-AMP pages only due to the absence of css/ampshared.css.
includes/assets/stories.css
Outdated
| /* Import 'glider-js' styles. */ | ||
| @import '../../node_modules/glider-js/glider.min.css'; |
There was a problem hiding this comment.
Note to myself: we really need some build tooling here for the CSS.
There was a problem hiding this comment.
Yes, I tried already but hit the roadblocks with some loader issues, I did set up basic sass build pipe but there were quite a few module build failures with imported components from the dashboard. Also there's an interesting issue with the webpack v4 and mini-css-extract-plugin when using multiple entries and chunks.
I resorted to removing the optimization for the webStoriesScript entry point, ( updating the PR ).
| @@ -340,6 +341,7 @@ protected function get_block_classes() { | |||
|
|
|||
| $block_classes = []; | |||
| $block_classes[] = 'web-stories-list'; | |||
| $block_classes[] = ( $this->is_amp_request() ) ? 'is-amp' : ''; | |||
There was a problem hiding this comment.
Instead of adding an is-amp class, the html element will have an amp attribute already, which you can target via CSS.
| } from '../../../dashboard/app/reducer/stories'; | ||
| import { ERRORS } from '../../../dashboard/app/textContent'; | ||
|
|
||
| const useStoryApi = (dataAdapter, { editStoryURL, storyApi }) => { |
There was a problem hiding this comment.
Why is editStoryURL needed here?
There was a problem hiding this comment.
editStoryURL is passed down to the storyReducer which returns the state with two data points
bottomTargetAction: `${editStoryURL}&post=${id}`,
editStoryLink: `${editStoryURL}&post=${id}`,
Though these are not required for our case, but it was returning urls with undefined parts. hence added it.
assets/src/web-stories-block/block/block-types/selected-stories/storyPicker.js
Outdated
Show resolved
Hide resolved
- Update: Comments and Component refactoring for the blocks.
- Removes redundant component structures.
- Updates web stories block to use pre defined fields state for default state of the different views. - Updates web stories block components to use field state, which is also an attribute for the block.
|
Merging this to the base gutenberg blocks branch for inclusive review. |
Summary
Updates Block API providers under
block/api.Though we now have separate Providers, the story data is being pulled by using the existing
storyReducerfrom the dashboard, as many components, we are using for the story picker modal rely on it and we don't want to create redundant copies.NOTE: This PRs is for the ideation and if the API provider changes looks good, will incorporate the same in main PRs.