Add ProductMenu, ProductMenuDropdown components. Add ProductMenuItems data. #14

danswick · 2018-07-13T22:29:39Z

closes #3

After chatting with @colleenmcginnis yesterday about the data that populates the ProductMenu, it seems clear that frontend-documentation would make a good home for the truth about which products should be in the menu. I included the most recent product list I could find in a new data directory. I'm not sure if that's the most sensible way to do it!

I'm also unsure whether it would be better to refactor ProductMenuDropdown a bit so it accepts a prop with all of the product data.

danswick · 2018-07-13T22:38:52Z

src/components/product-menu/product-menu.js

+  }
+
+  renderMenu() {
+    return <ProductMenuDropdown categories={ProductMenuItems} />;


Could we generalize this component a bit more by rendering ProductMenu's children instead of ProductMenuDropdown specifically?

edit: turns out "yes"

I'm not sure it needs to be generalized, really, unless we decide at some point that we want to repeat the button style with different content. For now, these components are coupled immediately with the product names themselves, and I think that's ok.

@davidtheclark 🙈 you're so fast! whelp, I rearranged the furniture a bit and I think I like it better this way. I don't know that we'll have an immediate use case for this style of button with other stuff inside of it, but I can see it being useful in the future.

Would you and @colleenmcginnis mind taking a peek at the new arrangement and let me know if you think it's worse?

davidtheclark

🥇 @danswick

davidtheclark · 2018-07-13T22:30:40Z

src/components/product-menu-dropdown/__tests__/product-menu-dropdown-test-cases.js

@@ -0,0 +1,15 @@
+import { ProductMenuDropdown } from '../product-menu-dropdown';


We should decide whether to favor default or named exports by default in the repo and stick with that. I'm fine with either way.

Per chat, we're going to stick with always named for now.

davidtheclark · 2018-07-13T22:31:28Z

src/components/product-menu-dropdown/product-menu-dropdown.js

+      position: 'absolute',
+      margin: '10px 0 0 -12px',
+      display: 'none'
+    };


Since this object doesn't get affected by the props, it could be declared outside the component's render method, which means we don't have to re-create the object on every render.

davidtheclark · 2018-07-13T22:32:49Z

src/components/product-menu-dropdown/product-menu-dropdown.js

+    const allCategories = this.props.categories.map((category, index) => {
+      const allProducts = category.products.map((product, index) => {
+        const liClasses = classnames('mt6 relative ', {
+          'txt-bold': product.url && location.pathname.indexOf(product.url) > -1


window.location won't be available when pages are rendered in Node, so there's a chance the current logic will break things during static HTML rendering.

davidtheclark · 2018-07-13T22:33:34Z

src/components/product-menu-dropdown/product-menu-dropdown.js

+          'txt-bold': product.url && location.pathname.indexOf(product.url) > -1
+        });
+        const dotClasses = classnames({
+          'inline-block': location.pathname.indexOf(product.url) > -1,


You could consolidate this logic into a single variable: isActive. Then you only have to figure it out once and can reapply it in these various contexts.

davidtheclark · 2018-07-13T22:34:17Z

src/components/product-menu-dropdown/product-menu-dropdown.js

+            <Icon
+              name={category.icon}
+              inline={true}
+              className="icon mb-neg6 mr6 h20 w20 color-gray inline"


Is inline necessary? Or is it covered by inline={true}?

@davidtheclark looks like they're both needed:

.inline sets display: inline !important

inline={true} does some math to position the icon vertically: https://github.com/mapbox/mapbox-react-components/blob/master/packages/react-icon/src/icon.js#L19-L28

davidtheclark · 2018-07-13T22:36:43Z

src/components/product-menu/product-menu.js

+          <Icon
+            name="caret-down"
+            inline={true}
+            className="flex-child fr icon h30 w30"


I think IE11 has problem with flex display on SVGs, so a wrapping div should take care of the layout instead of the Icon itself.

Why is this floating right?

Why is this floating right?

Similar to above, I'm not totally sure. Would you mind hopping in here, @colleenmcginnis?

davidtheclark · 2018-07-13T22:37:01Z

src/components/product-menu/product-menu.js

+        onPopoverClose={this.onPopoverClose}
+      >
+        <div className="fl wmax240-ml wmax180-mm flex-parent flex-parent--space-between-main flex-parent--center-cross">
+          <div className="flex-child inline-block txt-fancy txt-l cursor-pointer border-b border-b--2 border--white border--blue-on-hover txt-truncate">


flex-child and inline-block cancel each other out.

davidtheclark · 2018-07-13T22:37:58Z

src/components/product-menu/product-menu.js

+        onPopoverClose={this.onPopoverClose}
+      >
+        <div className="fl wmax240-ml wmax180-mm flex-parent flex-parent--space-between-main flex-parent--center-cross">
+          <div className="flex-child inline-block txt-fancy txt-l cursor-pointer border-b border-b--2 border--white border--blue-on-hover txt-truncate">


Curious why this div has a pointer cursor, instead of the higher-level container? I notice on the live site that if you hover over the caret icon you don't see the cursor, but you can click — probably we could improve on that.

davidtheclark · 2018-07-13T22:38:20Z

src/components/product-menu/product-menu.js

+        onPopoverClose={this.onPopoverClose}
+      >
+        <div className="fl wmax240-ml wmax180-mm flex-parent flex-parent--space-between-main flex-parent--center-cross">
+          <div className="flex-child inline-block txt-fancy txt-l cursor-pointer border-b border-b--2 border--white border--blue-on-hover txt-truncate">


border-transparent could be the default (non-hover), in case we ever put this on a non-white background.

davidtheclark · 2018-07-13T22:39:46Z

src/components/product-menu/product-menu.js

+
+ProductMenu.propTypes = {
+  platform: PropTypes.string.isRequired,
+  product: PropTypes.string.isRequired


@colleenmcginnis Is there any chance that this menu might contain non "SDK"s at some point? If so, maybe we want to remove the logic that forces an x SDK for y format, and instead allow products to be freeform strings?

Yes. We should change that — it will break for GL JS. @danswick let's just use a prop like productName to pass a string with the exact name we want to display.

colleenmcginnis

@danswick I left a couple lil comments in response to David's comments.

I also noticed that this is going beyond the bottom of the page. Is this something that will only be fixed with the custom CSS that you're going to add in a follow-up PR?

danswick · 2018-07-17T01:17:43Z

Thanks for 👀 @colleenmcginnis. Updated per your comments. Regarding the menu overrunning the bottom of the page, yes, I believe this will be resolved once our custom docs classes are added.

danswick added 2 commits July 12, 2018 18:14

add product menu and product menu dropdown components

da7673e

update product menu test case and switch to canonical product data

c4d5589

danswick requested review from colleenmcginnis and davidtheclark July 13, 2018 22:29

danswick commented Jul 13, 2018

View reviewed changes

davidtheclark reviewed Jul 13, 2018

View reviewed changes

danswick added 3 commits July 13, 2018 16:07

update product menu to accept arbirary elements as children prop

a193179

add location test

366f28e

update some style rules and remove unused no-render test cases

1f0bc24

colleenmcginnis reviewed Jul 16, 2018

View reviewed changes

address some CSS comments and generalize product names prop

7230fcf

danswick mentioned this pull request Jul 17, 2018

Add docs-specific CSS to this project #25

Closed

Merge branch 'master' into product-menu

1b419ad

danswick merged commit 88e3e7e into master Jul 17, 2018

danswick deleted the product-menu branch July 17, 2018 22:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ProductMenu, ProductMenuDropdown components. Add ProductMenuItems data. #14

Add ProductMenu, ProductMenuDropdown components. Add ProductMenuItems data. #14

danswick commented Jul 13, 2018 •

edited

Loading

danswick Jul 13, 2018 •

edited

Loading

davidtheclark Jul 13, 2018

danswick Jul 13, 2018

davidtheclark Jul 13, 2018

davidtheclark left a comment

davidtheclark Jul 13, 2018

danswick Jul 13, 2018

davidtheclark Jul 13, 2018

davidtheclark Jul 13, 2018

davidtheclark Jul 13, 2018

davidtheclark Jul 13, 2018

danswick Jul 16, 2018

davidtheclark Jul 13, 2018

davidtheclark Jul 13, 2018

danswick Jul 16, 2018

colleenmcginnis Jul 16, 2018

davidtheclark Jul 13, 2018

davidtheclark Jul 13, 2018

davidtheclark Jul 13, 2018

davidtheclark Jul 13, 2018

colleenmcginnis Jul 16, 2018

colleenmcginnis left a comment

danswick commented Jul 17, 2018

		@@ -0,0 +1,15 @@
		import { ProductMenuDropdown } from '../product-menu-dropdown';

Add ProductMenu, ProductMenuDropdown components. Add ProductMenuItems data. #14

Add ProductMenu, ProductMenuDropdown components. Add ProductMenuItems data. #14

Conversation

danswick commented Jul 13, 2018 • edited Loading

danswick Jul 13, 2018 • edited Loading

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

davidtheclark left a 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

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

colleenmcginnis left a comment

Choose a reason for hiding this comment

danswick commented Jul 17, 2018

danswick commented Jul 13, 2018 •

edited

Loading

danswick Jul 13, 2018 •

edited

Loading