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

Import primer-dropdown from github #393

Closed
wants to merge 26 commits into from
Closed

Conversation

jonrohan
Copy link
Member

@jonrohan jonrohan commented Nov 6, 2017

closes #338

This PR imports primer-dropdown as a new component. And refactors it to use our spacing scale as well as cleans up some class names.

TODO

  • Add primer-dropdown as a dependency of primer-product
  • Add @import "primer-dropdown/index.scss"; to primer-product/index.scss
  • Address Mu-An's feedback in Import primer-dropdown from github #393 (comment)
  • Update the base to the next release branch (10.11.0?)

@jonrohan jonrohan changed the base branch from release-10.0.0 to dev November 7, 2017 20:37
@jonrohan jonrohan added minor release and removed v10 labels Nov 7, 2017
@broccolini broccolini added the v10 label Nov 7, 2017
@jonrohan jonrohan removed the v10 label Nov 7, 2017
@jonrohan jonrohan changed the base branch from dev to release-10.1.0 November 15, 2017 17:28
@jonrohan jonrohan changed the title Refactor dropdown to use our variables Import primer-dropdown from github Nov 15, 2017
@jonrohan jonrohan changed the base branch from release-10.1.0 to dev November 16, 2017 19:43
@jonrohan jonrohan changed the base branch from dev to master June 25, 2018 19:40
@dmalan
Copy link

dmalan commented Jul 7, 2018

Hi all, just wanted to ask if there are (short-term) plans to merge dropdowns into master? (I see that they're listed as stable in https://styleguide.github.com/primer/components/dropdown/ but I gather that might be from when they were a separate repo?)

And if/when they are merged in, will Primer include the requisite JavaScript or would users of the library need to handle toggling?

Thanks!

@dmalan dmalan mentioned this pull request Jul 7, 2018
@shawnbot shawnbot changed the base branch from master to release-10.8.0 July 17, 2018 23:45
@shawnbot
Copy link
Contributor

I'm queueing this up for #531. @jonrohan, do you mind fixing up the version conflicts and taking a look at the comments above? 🙇

@emplums emplums mentioned this pull request Jul 17, 2018
15 tasks
Emily and others added 4 commits July 27, 2018 10:44
This reverts commit 61081cf.
 - primer-alerts@1.5.9
 - primer-avatars@1.5.6
 - primer-base@1.7.4
 - primer-blankslate@1.4.9
 - primer-box@2.5.9
 - primer-branch-name@1.0.7
 - primer-breadcrumb@1.5.5
 - primer-buttons@2.6.0
 - primer-core@6.10.3
 - primer-forms@2.1.4
 - primer-labels@1.5.9
 - primer-layout@1.4.9
 - primer-markdown@3.7.9
 - primer-marketing-buttons@1.0.8
 - primer-marketing-type@1.4.9
 - primer-marketing-utilities@1.6.5
 - primer-marketing@6.2.4
 - primer-navigation@1.5.7
 - primer-page-headers@1.4.9
 - primer-page-sections@1.4.9
 - primer-pagination@1.0.3
 - primer-popover@0.1.3
 - primer-product@5.6.6
 - primer-subhead@1.0.7
 - primer-support@4.6.0
 - primer-table-object@1.4.9
 - primer-tables@1.4.9
 - primer-tooltips@1.5.7
 - primer-truncate@1.4.9
 - primer-utilities@4.12.0
 - primer@10.7.1
 - generator-primer-module@1.0.8
 - primer-module-build@1.0.5
 - stylelint-config-primer@2.2.10
 - stylelint-selector-no-utility@1.8.10
@muan
Copy link
Contributor

muan commented Jul 27, 2018

I wasn't sure if it made sense to keep the js-bulk-actions, and js-transitionable classes in the log out menu example. I think the js-menu-close classes can go since I've killed off the js-menu-container though, right? I'm thinking that for the purposes of the style guide it might make sense to remove all of the js- classes since they aren't going to work in the style guide anyway. What do you think?

Yup! js-bulk-actions, js-transitionable should go since they have ~0 to do with the menu functionality.

However, on <details> the js-menu-close behavior is now either handled by <details-menu> or done throughdata-toggle-for="explict-id-on-details-tag". Given this..... I am changing my mind regarding whether to include <details-menu>. I think we should 🤔 , at least for one example.

I see style guide as a place people can find a fully functioning + clean code snippet that they would just edit and go. Copy/pasting from the codebase are likely to result in a lot of unnecessary markup/js/css class duplication. And in this case, without details-menu it's kinda of not fully functioning.

What do you think?

@shawnbot
Copy link
Contributor

@muan thanks, I think that makes a ton of sense. I just need to get the custom element bundles into ours before I can make that work.

@emplums emplums mentioned this pull request Oct 15, 2018
16 tasks
@shawnbot shawnbot changed the base branch from release-10.8.0 to release-10.9.0 October 17, 2018 20:39
@shawnbot shawnbot mentioned this pull request Oct 22, 2018
19 tasks
@shawnbot shawnbot changed the base branch from release-10.9.0 to release-10.10.0 October 23, 2018 21:23
@shawnbot
Copy link
Contributor

@jonrohan I'm inclined to file an issue for updating the docs so that we can get this out in #588. What do you think?

@shawnbot shawnbot changed the base branch from release-10.10.0 to master November 20, 2018 00:54
@shawnbot
Copy link
Contributor

These styles are in the process of being refactored in dot-com, so I'm going to open a new PR with some fresh CSS when it's ready to bring into the fold. Thanks for your feedback, @muan!

@shawnbot shawnbot closed this Apr 15, 2019
@shawnbot shawnbot deleted the variablize_dropdown branch April 15, 2019 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update dropdown component to use variables
6 participants