Skip to content
This repository was archived by the owner on Nov 19, 2024. It is now read-only.

Add Layout XML Standard (new branch) #1078

Closed
wants to merge 2 commits into from

Conversation

bassplayer7
Copy link
Contributor

The goal of this is to formalize a standard for writing layout XML. There hasn't been a standard set that the community follows when authoring layout XML. By setting a precedent for layout XML naming conventions, I am hopeful that it will lead to XML declarations that are easier to read and more consistent which could decrease maintenance time.


Note: this is identical to #982. I found that I did that PR on the wrong branch of my fork and needed to move over to this one to provide flexibility.

@ghost ghost assigned almarchenko Mar 26, 2017
@almarchenko
Copy link
Contributor

Hi @bassplayer7!

Thank you for your input!
We have the "name" and "as" attributes described in the layout-related chapter of Frontend Developer Guide:
http://devdocs.magento.com/guides/v2.1/frontend-dev-guide/layouts/xml-instructions.html#fedg_layout_xml-instruc_ex_block
But in fact, of course, they are useful not only for frontend developers. And also your descriptions contain more info.
I've created an internal ticket (67490) to consult with our internal developers.

@almarchenko almarchenko added the Tracking Created an internal Jira ticket to track work label Apr 14, 2017
@almarchenko
Copy link
Contributor

@bassplayer7 What I found out is that currently we don't have these internal standards.
We need the opinion and decision of architects on this.
@antonkril perhaps you can help? Should Magento start following these standards?

@antonkril
Copy link
Contributor

@bassplayer7, this is great. We'll discuss it internally.

@antonkril antonkril self-requested a review June 4, 2017 15:33
@bassplayer7
Copy link
Contributor Author

@almarchenko thanks for checking into this.

@antonkril thanks for picking this up as well. The exact details don't much matter to me, but over time, I've felt that having at least a naming recommendation could help us identify blocks more instinctively.

Copy link
Contributor

@antonkril antonkril left a comment

Choose a reason for hiding this comment

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

Added discussion comments.

Resolution:

  • We agree that we need the standard for this.
  • Concerns with the proposal were identified

Next steps:

  • @benmarks will drive the proposed solutions to concerns
  • The proposed solutions will be merged to the proposed standard and published


The `name` attribute should adhere to the guidelines listed below. The `name` attribute should:

- Be unique to the project.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can not be enforced without a more specific rule (add namespace?)

The `name` attribute should adhere to the guidelines listed below. The `name` attribute should:

- Be unique to the project.
- Use a namespace approach where each child block's name contains its parent's name as well.
Copy link
Contributor

Choose a reason for hiding this comment

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

Cons:

  • @benmarks : it is a duplication of the structure. Layout structure can be modified ()
  • @okorshenko : reusable declarations (renderer lists) will not be able to follow this approach
  • will mix structure of blocks and multi-word block names (example: i want to call my bllock "my.block")

Pros:

  • @buskamuza : will allow to see the original place of a block.
  • @okorshenko : improves unique block naming

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding for the record (this is the salient point):

Use a namespace approach where each child block's name contains its parent's name

Because block names exist in the global layout scope, this isn't a "namespace approach"; rather, it's a hierarchy approach.


```xml
<block name="header">
<block name="header.right">
Copy link
Contributor

Choose a reason for hiding this comment

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

"Name" should not describe the position of a block. It should explain its purpose. We should add it as one of the rules.


The `as` attribute should follow the guidelines below. The value of `as`:

- Only needs to be unique to the block that contains it.
Copy link
Contributor

Choose a reason for hiding this comment

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

@benmarks : With current implementation, this one can be confusing for block substitution scenario (when we add a block with same alias to substitute a other child)

Copy link
Contributor

Choose a reason for hiding this comment

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

I have an internal goal of having this complete tonight (12-Jun), but it will be a couple more days. Thanks again for proposing.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do we expect in this scenario? I'd leave the statement as is.

magento-cicd2 pushed a commit that referenced this pull request Aug 11, 2017
Add PayPal to navigation - corrected for deploy work
@jcalcaben
Copy link
Contributor

@benmarks @antonkril
what is the status of this PR? is it still in development or ready to merge?

@buskamuza buskamuza mentioned this pull request Jun 1, 2018
3 tasks
@buskamuza
Copy link
Contributor

Created another PR as I can't edit this one - #2051

@benmarks
Copy link
Contributor

benmarks commented Jun 2, 2018

I haven't gotten to this in a year, so consider me removed.

@jcalcaben jcalcaben changed the base branch from develop to master July 6, 2018 17:40
@buskamuza
Copy link
Contributor

Closing the proposal based on the discussion https://github.com/magento/architecture/wiki/October-31,-2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Tracking Created an internal Jira ticket to track work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants