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

Update Layout implementation so the initial element is the root element #1356

Closed
tleunen opened this issue Aug 11, 2015 · 13 comments
Closed

Comments

@tleunen
Copy link
Contributor

tleunen commented Aug 11, 2015

I'm trying to implement MDL with React, using mostly what you already did so it's easier to maintain but I'm facing a few issues (tleunen/react-mdl#4 for example).

Base on my tests, it seems most of the issues came from components where the initial node moves in the tree. With the layout component, the initial node mdl-layout moves inside the automatically created mdl-layout__container.

Would it be possible to change the implementation so the initial node is the container itself, and creates a new inner node and moves all the children in the new node?

I'm testing a few things based on that right now, it doesn't seem to be a lot of work or a lot of changes. Mostly a few things to adapt in the CSS and the layout__container would become a layout__inner-container for example.

@sgomes
Copy link
Contributor

sgomes commented Aug 11, 2015

Thanks, @tleunen, this is a very good suggestion. The whole mdl-layout__container situation is definitely something we want to fix with a v2.

Sadly, we can't do anything until then, as this is a hard BC break :-/ But thank you for the PR, we'll definitely use it as a starting point whenever we do begin development on a v2! It'll have to sit quietly in the queue for a while, though :)

@JacobDorman
Copy link
Contributor

+1 - i'd like to see mdl-layout__container go.
Browser-sync's scrolling fails, as do screenshot services like crossbrowsertesting.com. I assume because they attempt to scroll the body, which contains an absolutely positioned block with 100% width/height.

I haven't looked into it very deeply, as those things aren't too important... but search indexing is a concern. Can headless browsers like googlebot navigate a mdl-layout__container wrapped page?

@sgomes
Copy link
Contributor

sgomes commented Sep 28, 2015

That's definitely the goal for eventual work in v2, @JacobDorman. Personally, I'd like to reimplement mdl-layout in v2 in a more modular fashion, with fewer prescribed behaviours and a simpler (and less strict) DOM structure.

@JacobDorman
Copy link
Contributor

Just FYI, i tested with phantomjs and then Google Search Console's 'fetch and render' ... neither were able to scroll.

I haven't tested the example site yet, so it could be something to do with my implementation. Does anyone have access to test http://www.getmdl.io/ in GSC?
... or since it says google at the top of the page, maybe someone could just wander over to @mattcutts cubicle (I'm just going to assume google has a layout similar to my office)

If there is a way to get a comment from the search quality team that would be great though.

@JacobDorman
Copy link
Contributor

FWIW
Initial View
www_getmdl_io_--1
Scroll 512px
www_getmdl_io_--2

Initial View
image
Scroll 512px

en_wikipedia_org_wiki_material_design--2

https://gist.github.com/a0c7e6d468f7794b3ce1

@cadesalaberry
Copy link

Would love to see this behavior normalised as well !

@quantuminformation
Copy link

+1

@sgomes
Copy link
Contributor

sgomes commented Jan 4, 2016

As an update for everyone on this thread, we'll be making layout more modular in 2.x, which will mean less prescribed behaviour and more customization. This will allow us to get rid of mdl-layout__container. Stay tuned!

@nha
Copy link

nha commented Jan 31, 2016

Hello,

I just started integrating it in my react app (well om.next app) and see no trouble so far but before going any further I would like to know :

  • should I use mdl in a react project now, or should I wait ?
  • In case I can, any restrictions ? Like having to call upgradeElement on every element when rendering ?

@tleunen
Copy link
Contributor Author

tleunen commented Jan 31, 2016

@nha You can take a look at how I did it with react-mdl, but basically yes, you have to call the upgrade/downgrade functions

@traviskaufman
Copy link
Contributor

Closing as we will be splitting up layout into multiple components in v2, for example sidenav (#4476) and toolbar (#4480). We will no longer be automatically inserting or modifying any DOM, especially ones that deal with major portions of the page. Hopefully this helps to ameliorate this issue.

We are still in the very early stages of v2 and are currently working towards an alpha. Our master branch is where all v2 work is happening if you are interested in keeping up with its progress. 😄

@tleunen
Copy link
Contributor Author

tleunen commented Jul 12, 2016

@traviskaufman Would it be a way for us to already test some of your v2 components? What would be the best way to do that?

@Garbee
Copy link
Collaborator

Garbee commented Jul 12, 2016

What we currently have is in master however it is under very heavy flux. We also are not providing support for working with master until we hit alpha. So explore master at your own risk currently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants