Skip to content
This repository was archived by the owner on Dec 11, 2021. It is now read-only.

New Component: Modals #163

Closed
wants to merge 1 commit into from

Conversation

nashvail
Copy link
Contributor

No description provided.

@jquerybot
Copy link

Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA).

📝 Please visit http://contribute.jquery.org/CLA/ to sign.

After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know.


If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check.

@cvrebert
Copy link
Contributor

I suggest including an example where the modal's content is taller than the viewport (and thus, scrolling is required).


.modal-header {
@include modal-header();
h4 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardcoded tag selector :-/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default one as styled in typography.scss has wide top and bottom margins so I had to override that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about a .modal-title class then?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general we want to avoid cascading as well. Have modal-title so they can use whatever element they need semantically, and remove modal-header cascading from inside modal.

Sent from my iPhone

On May 27, 2016, at 4:43 PM, Chris Rebert notifications@github.com wrote:

In scss/atoms/modals/_modals.scss:

+* ==========================================================================
+* Modals
+* ==========================================================================
+*/
+
+@import

  • "dist/chassis",
  • "functions",
  • "mixins";

+.modal {

  • .modal-header {
  •   @include modal-header();
    
  •   h4 {
    
    What about a .modal-title class then?


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@sfrisk
Copy link
Contributor

sfrisk commented Jun 1, 2016

button stacking is a little awkward in mobile.

@@ -0,0 +1,5 @@
body {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for something like this, maybe a common demo.css file would be better, than separate stylesheets for everything?

@sfrisk sfrisk added this to the Phase Two milestone Jun 14, 2016
@sfrisk sfrisk closed this Feb 14, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants