✨ [bento][amp-accordion] CSS Port draft for amp accordion (PR #2) #30622

krdwan · 2020-10-12T16:15:54Z

Partial #30445.

Having a problem with rebasing on the previous PR (#30593), so created a new PR instead.

Porting CSS over from 0.1.

Added styling for bento and amp modes for header and content sections.
Preact mode is controlled by JSS
amp mode is controlled by component css styling

CLAassistant · 2020-10-12T16:15:59Z

All committers have signed the CLA.

dvoytenko · 2020-10-12T18:04:22Z

extensions/amp-accordion/1.0/accordion.js

@@ -48,6 +48,13 @@ const CHILD_STYLE = {
  position: 'relative',
 };

+const SECTION_HEADER_STYLE = {
+  cursor: 'pointer',
+  backgroundColor: '#efefef',


Cursor pointer is ok here. But we can't put any of the rest into the inline style - this would make them un-overridable in stylesheets. Let's remove these styles for now. If the wg decides we want these styles here, we will have to bring JSS back. But for now it's easier to not have it, imho.

dvoytenko · 2020-10-12T18:04:52Z

extensions/amp-accordion/1.0/accordion.js

@@ -152,6 +159,8 @@ export function AccordionSection({
  contentAs: ContentComp = 'div',
  expanded: defaultExpanded = false,
  animate: defaultAnimate = false,
+  headerStyle = {},


No need for defaults here.

Thanks, removed.

dvoytenko · 2020-10-12T18:05:52Z

extensions/amp-accordion/1.0/amp-accordion.css

+}
+
+/* heading
+ * TODO(dvoytenko): remove most of these styles, except maybe for `cursor`.


Rewrite TODO as TODO(#30445): ....

dvoytenko · 2020-10-12T18:06:50Z

extensions/amp-accordion/1.0/amp-accordion.js

@@ -128,6 +129,7 @@ const bindSectionShimToElement = (element) => SectionShim.bind(null, element);
 */
 function HeaderShim(sectionElement, {onClick}) {
  const headerElement = sectionElement.firstElementChild;
+  headerElement.classList.add('i-amphtml-accordion-header');


This is a side-effect. Let's move it to the useLayoutEffect below. Alternatively, we can move this code to getState.

dvoytenko · 2020-10-12T18:07:01Z

extensions/amp-accordion/1.0/amp-accordion.js

@@ -157,6 +159,7 @@ const bindHeaderShimToElement = (element) => HeaderShim.bind(null, element);
 */
 function ContentShimWithRef(sectionElement, {hidden}, ref) {
  const contentElement = sectionElement.lastElementChild;
+  contentElement.classList.add('i-amphtml-accordion-content');


krdwan · 2020-10-12T21:13:34Z

Latest revision includes:

Moved styling in Preact to use JSX per team discussion.
This includes CHILD_STYLING(CSS related to animations)
Addressed review comments.

dvoytenko

Looks good, but one request.

dvoytenko · 2020-10-14T00:00:13Z

extensions/amp-accordion/1.0/accordion.js

@@ -37,17 +38,6 @@ const EMPTY_EXPANDED_MAP = {};

 const generateSectionId = sequentialIdGenerator();

-const CHILD_STYLE = {


These particular styles are not overridable. As such, we can continue to inline them. If you still want them in the JSS - it's totally reasonable, but we need to add !important.

I personally prefer using JSS to keep all styling together. I think it improves readability so the developer can quickly look into the one JSS file to see all styling. I have added !important. If there's something else I should consider, don't hesitate to let me know :). Thanks!

dvoytenko

LGTM with one ask for tests.

dvoytenko · 2020-10-15T00:29:19Z

extensions/amp-accordion/1.0/test/test-accordion.js

+      expect(content2.hidden).to.be.true;
+
+      // Styling.
+      expect(header0.className.includes('section-child')).to.be.true;


IMHO instead of testing for class names (or with them) I'd like to attach the accordion to DOM and call getComputedStyle() to check critical styles - the ones we marked with !important. Otherwise, the class presence does not guarantee that we didn't make a bug with the class definition itself.

Sounds good Dima. For the test-amp-accordion.js (amp mode tests) I have made the updates.

For test-accordion.js (preact tests), I have not been able to get the tests to work. A few things I tried include:

Attach the component DOM node to its own ownerDocument.defaultView (see lines 115 - 117). However, I don't think the style tag from JSS is installed here, so not getting the styles as expected.

I also tried using options in the mount section (see line 109). However, I'm unable to attach the component DOM node as win.document is apparently undefined.

I'm not too familiar with the scoping in the test environments. Do you have any recommendations on where I can find out more about this?

code lines refer to latest commit: d08c4fb
(file: test-accordion.js)

While these tests are super thorough and help increase confidence, it's not common to test CSS properties this granularly inside the components for visual styles. If it's not too much trouble, what do you think about copying the visual tests from 0.1 over? It should give us just as much information without as much of this tedium.

Thanks! As discussed offline, I've added an item in the tracker issue to look into the visual tests separately from this PR. #30445

caroqliu · 2020-10-16T17:46:20Z

extensions/amp-accordion/1.0/accordion.jss.js

+};
+
+// TODO(#30445): update these styles after team agrees on styling
+// or delete is not used


Suggested change

// or delete is not used

// or delete if not used

caroqliu · 2020-10-16T17:49:10Z

extensions/amp-accordion/1.0/accordion.js

+  headerClassName,
+  contentClassName,


nit: What do you think about giving these a default value with headerClassName = '' so we do not need the || '' later on?

I'm fine with this, updated.

caroqliu · 2020-10-16T17:57:51Z

extensions/amp-accordion/1.0/amp-accordion.css

+/* heading/content */
+.i-amphtml-accordion-header,
+.i-amphtml-accordion-content {
+  margin: 0;


This is actually interesting. It looks like in AMP, we don't respect margins of a given header, but in Preact we do by nature of wrapping the given header element inside another layer of <header> tag. @dvoytenko Is there a reason we did not attach the header class/handlers/etc on the cloneElement of the given header in Preact? Also why did we move away from the accordion model of "first child is header and second child is content"?

I understand these questions are not directly related to the PR so feel free to consider them non blocking here.

caroqliu · 2020-10-16T18:02:16Z

extensions/amp-accordion/1.0/test/test-accordion.js

+      expect(content2.hidden).to.be.true;
+
+      // Styling.
+      expect(header0.className.includes('section-child')).to.be.true;


While these tests are super thorough and help increase confidence, it's not common to test CSS properties this granularly inside the components for visual styles. If it's not too much trouble, what do you think about copying the visual tests from 0.1 over? It should give us just as much information without as much of this tedium.

…ect#2) (ampproject#30622) * Update CSS for accordion * Add JSX into Preact component styling and misc review comments * Add important to make section styles impossible to overwrite * Add tests for CSS in amp-accordion * Accordion tests updates * Removed tests for getComputedStyle * Remove debugging comments * Address nits from review

Update CSS for accordion

9d44710

google-cla bot added the cla: yes label Oct 12, 2020

krdwan changed the title ~~Update CSS for accordion~~ ✨ [bento][amp-accordion] CSS Port draft for amp accordion Oct 12, 2020

krdwan mentioned this pull request Oct 12, 2020

[DUPLICATE] ✨ [bento][amp-accordion] CSS Port draft for amp accordion #30593

Closed

krdwan changed the title ~~✨ [bento][amp-accordion] CSS Port draft for amp accordion~~ ✨ [bento][amp-accordion] CSS Port draft for amp accordion (PR #2) Oct 12, 2020

dvoytenko reviewed Oct 12, 2020

View reviewed changes

Add JSX into Preact component styling and misc review comments

d7833c7

krdwan mentioned this pull request Oct 13, 2020

[amp-accordion:1.0] Bento tracking issue #30445

Closed

20 tasks

dvoytenko suggested changes Oct 14, 2020

View reviewed changes

krdwan added 2 commits October 14, 2020 12:28

Add important to make section styles impossible to overwrite

04dc75b

Add tests for CSS in amp-accordion

3bc6dd9

dvoytenko approved these changes Oct 15, 2020

View reviewed changes

krdwan added 3 commits October 15, 2020 11:51

Accordion tests updates

d08c4fb

Removed tests for getComputedStyle

22cb849

Remove debugging comments

8489ed5

krdwan marked this pull request as ready for review October 15, 2020 19:23

caroqliu approved these changes Oct 16, 2020

View reviewed changes

krdwan and others added 2 commits October 19, 2020 10:21

Address nits from review

97718a6

Merge branch 'master' into amp-accordion-css2

6f3d979

krdwan merged commit 21d8e50 into ampproject:master Oct 20, 2020

ampprojectbot added the PR use: In Beta / Experimental label Oct 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ [bento][amp-accordion] CSS Port draft for amp accordion (PR #2) #30622

✨ [bento][amp-accordion] CSS Port draft for amp accordion (PR #2) #30622

krdwan commented Oct 12, 2020 •

edited

CLAassistant commented Oct 12, 2020 •

edited

dvoytenko Oct 12, 2020

dvoytenko Oct 12, 2020

krdwan Oct 12, 2020

dvoytenko Oct 12, 2020

krdwan Oct 12, 2020

dvoytenko Oct 12, 2020

krdwan Oct 12, 2020

dvoytenko Oct 12, 2020

krdwan Oct 12, 2020

krdwan commented Oct 12, 2020

dvoytenko left a comment

dvoytenko Oct 14, 2020

krdwan Oct 14, 2020

dvoytenko left a comment

dvoytenko Oct 15, 2020

krdwan Oct 15, 2020 •

edited

caroqliu Oct 16, 2020

krdwan Oct 20, 2020

caroqliu Oct 16, 2020

krdwan Oct 19, 2020

caroqliu Oct 16, 2020

krdwan Oct 19, 2020

caroqliu Oct 16, 2020

caroqliu Oct 16, 2020

		@@ -37,17 +38,6 @@ const EMPTY_EXPANDED_MAP = {};

		const generateSectionId = sequentialIdGenerator();

		const CHILD_STYLE = {

✨ [bento][amp-accordion] CSS Port draft for amp accordion (PR #2) #30622

✨ [bento][amp-accordion] CSS Port draft for amp accordion (PR #2) #30622

Conversation

krdwan commented Oct 12, 2020 • edited

CLAassistant commented Oct 12, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krdwan commented Oct 12, 2020

dvoytenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krdwan Oct 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krdwan commented Oct 12, 2020 •

edited

CLAassistant commented Oct 12, 2020 •

edited

krdwan Oct 15, 2020 •

edited