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

amp-accordion URL-based state initialization #16110

Closed
jamesshannon opened this issue Jun 18, 2018 · 8 comments
Closed

amp-accordion URL-based state initialization #16110

jamesshannon opened this issue Jun 18, 2018 · 8 comments

Comments

@jamesshannon
Copy link
Contributor

jamesshannon commented Jun 18, 2018

This is somewhat similar to my other requests for URL-based state (for, e.g., carousel).

The use case is that there are a handful of accordions and each represents a different state on the page and should be configurable based on the URL (query param or hash). Based on a Google search this appears to be a pretty standard practice for accordions.

Without this the client plans on creating an AMP page and then duplicating it x times (for each accordion), so they'd have page-optionA.html, page-optionB.html, etc.

This request could be seen as two parts, which I break out because #1 is more urgent for the client right now.

  1. amp-accordion should be able to initialize state from the URL.

I see that amp-accordion can initialize from a cookie state, so it seems that adding this would be pretty straightforward. I would suggest:

  • Uses hash param instead of URL param
  • Doesn't try to cram the JSON object that the cookie uses into the hash param, but instead uses a "sparse" form that can be parsed out, in which only the open accordions need to be set
  • Disambiguates with the accordion ID in the hash param
  • Operates automatically; does not require an additional attribute in the accordion
  • Where cookie state is enabled this will override that.

So it'd look like:
page.html#accordion_id-opened=pane1,pane5

  1. Longer term, along with other URL-based state updating requests, it'd make sense to allow for the update the URL hash when the accordions panes are opened or closed.
@aghassemi
Copy link
Contributor

amp-carousel version of this: #12659

@aghassemi
Copy link
Contributor

/cc @cathyxz

Adding an expanded=<id1,id2,..> attribute to amp-accordion and allowing variable substitution can also be a short term solution here.

Whatever we do here will cause content shift unless it gets applies in v0.js ( even in that case, it will cause content shift in SSR-case unless integrated there as well ). Having the logic in v0 of course increases the side and SSR-integration increases complexity. So some impact analysis is needed before we make this change. Similar to other dynamic features, we will make the decision case-by-case if it endsup breaking AMP's UX principles.

@jamesshannon
Copy link
Contributor Author

Wouldn't the content shift happen due to the cookie-based session state as well?

Not sure what you mean by variable substitution, but I did try playing with an [expanded] attribute and that wouldn't validate. Even if it did validate successfully I'd be concerned that it wouldn't execute quickly. ... But you might be thinking of something different.

@aghassemi
Copy link
Contributor

@jamesshannon It does happen with cookie-based session state already unfortunately.

variable substitution is faster than amp-bind and unlike bind it runs on page load. amp-accordion does not support it, but if we take that approach it would look like amp-accordion expanded="QUERY_PARAM(accordion_id-opened)"

One solution to the UX problem is doing something similar to resizing and deny the request if it causes jumps. With prerender, it would almost never be denied, without prerender, if below the fold it would work as well. That should cover vast majority of cases.

@ampprojectbot
Copy link
Member

This issue hasn't been updated in awhile. @aghassemi Do you have any updates?

1 similar comment
@ampprojectbot
Copy link
Member

This issue hasn't been updated in awhile. @aghassemi Do you have any updates?

@morsssss
Copy link
Contributor

I've seen this requested a couple of times... I think there's even another issue for this.

@nainar , do you think this could only be implemented in a way that caused content to shift? Or is there another way?

@stale
Copy link

stale bot commented May 13, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale Inactive for one year or more label May 13, 2021
@stale stale bot closed this as completed May 22, 2021
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

5 participants