-
Notifications
You must be signed in to change notification settings - Fork 115
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
Pulls/4.0/transmutation #443
Pulls/4.0/transmutation #443
Conversation
d6d66be
to
8627186
Compare
@@ -3,6 +3,7 @@ import { Button, Input, InputGroup, InputGroupAddon, Popover } from 'reactstrap' | |||
import classNames from 'classnames'; | |||
import { elementTypeType } from 'types/elementTypeType'; | |||
import i18n from 'i18n'; | |||
import addElementMutation from 'state/editor/addElementMutation'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We normally apply these HOC's with Injector. This allows people to sub out this query for their own if need be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be fair, this pattern is how we've done it before, but we should be aiming to make them injectable where possible. Related: silverstripe/silverstripe-admin#611
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was implemented as directed, however have updated it anyway as I also think it's a good idea.
@@ -22,7 +35,7 @@ class ElementEditor extends PureComponent { | |||
ElementEditor.propTypes = { | |||
elementTypes: PropTypes.arrayOf(elementTypeType).isRequired, | |||
pageId: PropTypes.number.isRequired, | |||
baseAddHref: PropTypes.string.isRequired, | |||
elementalAreaId: PropTypes.oneOfType([PropTypes.number, PropTypes.string]).isRequired, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice to have some better type safety on this prop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be nice to have return types be consistent, yes. I may be confusing my JS and PHP here though, is it always an Int in React? I've seen errors in both systems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can cast it before we inject the prop if you want to just make it number
. IDs in SilverStripe can be strings or ints in different environments =D
Actually if this comes from a GraphQL query then we may not be able to do this gracefully. This is what the GraphQL schema says about the ID type:
The ID type appears in a JSON response as a String; however, it is not intended to be human-readable. When expected as an input type, any string (such as "4") or integer (such as 4) input value will be accepted as an ID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not certain of the outcome on this comment :/
Could you please clarify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you can control the input (e.g. from entwine hydration) then cast it to a number and make this strict, if you can't control it then leave it like this =)
@@ -97,7 +97,7 @@ class HoverBar extends Component { | |||
'element-editor__add-block-hover-bar', | |||
{ | |||
'element-editor__add-block-hover-bar--delay': !calledInstantaneously, | |||
'element-editor__add-block-hover-bar--inst': calledInstantaneously, | |||
'element-editor__add-block-hover-bar--inst': calledInstantaneously |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change isn't required - it's arguably nicer to leave trailing commas if they're supported - smaller diffs means less chances to conflict!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be great if usage was consistent throughout the system too, and this change is trying to bring things in line with existing code ... or maybe it's an auto-format thing... (this comes up often particularly where e.g. my linter formatter goes and makes edits that are apparently contrary to someone else's... I already stripped/re-reset a bunch before making this PR).
Is there any guidance on this as to which way we desire such style that I'm particularly unaware of?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linters that automatically change existing code are very dangerous!! I've wasted a lot of time with peer reviews that have boiled down to automated changes. We should keep this kind of change to separate PRs (as mentioned above), where we can say "OK, it's automated and all LGTM so merge"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might be from an earlier formatting, as the PR is 2 commits and I don't recall seeing this pop up (I corrected others).
🤔 I wonder if I can configure the linter to do one style over the other/ignore the presence/absence of trailing commas.
(AddElementPopoverComponent) => ({ | ||
AddElementPopoverComponent, | ||
AddElementPopoverComponent => ({ | ||
AddElementPopoverComponent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing comma should be left in.
ID, | ||
Sort, | ||
IsPublished, | ||
IsLiveVersion, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These commas aren't needed. I actually thought GraphQL kicks up a fuss but apparently not 🤔 . Additionally you're doing a refetch so you can technically omit all return values.
Alternatively you can update the return values to match readBlocksForPage
and then you can do an update on the store after the values are returned. See the sort mutation: https://github.com/dnadesign/silverstripe-elemental/blob/master/client/src/state/editor/sortBlockMutation.js#L28. The optimisticResponse
is not relevant because you can't assume the response, but you should be able to splice your item into the known list of blocks.
Essentially here you're reading the cached response of a GraphQL query and updating that response. Apollo will automatically update consumers of that query.
This will drop a request to the server (the refetch of the query)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So turns out commas are OK.
So are line returns.
So are spaces.
So are combinations of all of the above 😑
Personally I prefer the style being more in-line with the JSON that'd be returned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They might be OK but in lieu of a GraphQL query "coding standard" we should at least aim for uniformity among the queries in core and the wider ecosystem (no commas)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so we looked at this together, however as the pageId
is required we (@ScopeyNZ and I) thought it better to wait for the refactor from getElementsForPage
to become getElementsForArea
- we already have the ElementalAreaID
, and re-inserting the PageID to filter all the way through all areas seems... unnecessary (read: tech debt that we'd have to remove later).
|
||
// GraphQL query for deleting a specific block | ||
const mutation = gql` | ||
mutation AddBlock($className: String!, $elementalAreaID: ID!, $afterElementID: ID) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mutation should be called AddElement
or maybe AddElementToArea
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should.
@@ -22,7 +35,7 @@ class ElementEditor extends PureComponent { | |||
ElementEditor.propTypes = { | |||
elementTypes: PropTypes.arrayOf(elementTypeType).isRequired, | |||
pageId: PropTypes.number.isRequired, | |||
baseAddHref: PropTypes.string.isRequired, | |||
elementalAreaId: PropTypes.oneOfType([PropTypes.number, PropTypes.string]).isRequired, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can cast it before we inject the prop if you want to just make it number
. IDs in SilverStripe can be strings or ints in different environments =D
Actually if this comes from a GraphQL query then we may not be able to do this gracefully. This is what the GraphQL schema says about the ID type:
The ID type appears in a JSON response as a String; however, it is not intended to be human-readable. When expected as an input type, any string (such as "4") or integer (such as 4) input value will be accepted as an ID.
@@ -97,7 +97,7 @@ class HoverBar extends Component { | |||
'element-editor__add-block-hover-bar', | |||
{ | |||
'element-editor__add-block-hover-bar--delay': !calledInstantaneously, | |||
'element-editor__add-block-hover-bar--inst': calledInstantaneously, | |||
'element-editor__add-block-hover-bar--inst': calledInstantaneously |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linters that automatically change existing code are very dangerous!! I've wasted a lot of time with peer reviews that have boiled down to automated changes. We should keep this kind of change to separate PRs (as mentioned above), where we can say "OK, it's automated and all LGTM so merge"
AddElementPopoverComponent, | ||
elementTypes, | ||
elementId, | ||
elementalAreaId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here'd be good to add a trailing comma so this type of diff doesn't re-occur
4452874
to
6cb2891
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked it out and had a look too. Works great 🙂 . There is an issue I know @clarkepaul would pick up on - that the blocks look a bit weird when there's no title or content. I had a super quick (5 minute) chat with him about it and he said we should have a default title, italicised: "Untitled {type} block" where {type}
is the block type (ie. Content, Banner etc...).
Additionally, the blocks should be the same height even if there's no content. A min-height of 1em might need to be added for the summary section or something.
4c8e3b1
to
bfefd04
Compare
Final behat test failure appears unrelated - working with @ScopeyNZ to resolve (over multiple development branches). |
bfefd04
to
8766197
Compare
GREEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEN 💚 💜 ? |
@NightJar I merged another PR which it'd be good for you to resolve the conflicts on and re-test, I think it was #439 which also changes from pulling blocks for a page to pulling blocks for a page's area. I approve the PR as it stands, and am happy for you to self merge it in the morning once it's green. |
Cool, it'll be needing a rebase since another merge - I'll do that then merge when green :) |
In order to create new blocks through the new 'inline' editing user interface, we first must have the ability to call the GraphQL query to create the block.
As part of inline editing for Elemental it has previously been only possible to edit existing elements. Now it is possible to add new ones to the relevant list for the pop over - both the add button above the list, or the in-between button that pops up on hovering the area close to the junction between two elements in the list. For this work it was necessary to expose the ID of the elemental area they belong to, in order to ensure elements are added to it as children. There have been a few scss linting issues tidied up also.
Previously when adding an element to an elemental area it would default to having no title, and no content (of any type). They now are given some filler text for a title, at a different shade to indicate the fact that it is not a 'real' title, and all elements have a min-height... not by explictly setting this, but by always drawing the content area summary paragraph (even if it's empty). This also solves the case of when there is only one element in the area, one could see the area 'sticking out' from behind the element. Because the element is now larger (mostly padding and margins) this no longer happens.
Recent changes to the way that blocks are created and inserted into an elemental area mean that the granularity of the behat tests was causing them to fail. Changes here are still reasonably granular, but a step in simplifying to BDD
8766197
to
a280eca
Compare
Neat, thanks team :) |
Resolves #340
Resolves #294
Allows a user to add an element without having to browse to a new form in order to fill in default information. This allows speedier content entry and reinforces inline editing as the standard way to edit elements.