Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

D8CORE-1261: Revised table styles. #617

Merged
merged 6 commits into from
Oct 7, 2020
Merged

Conversation

sherakama
Copy link
Member

@sherakama sherakama commented Feb 7, 2020

READY FOR REVIEW

Summary

  • Updates default styles for all tables

Needed By (Date)

  • At your leisure.

Urgency

  • None

Steps to Test

  1. Review code
  2. Click tugboat and review elements page for table styles

See Also

@sherakama sherakama temporarily deployed to Tugboat February 7, 2020 22:54 Destroyed
@sherakama
Copy link
Member Author

We may need a page of all html elements to review this.

@yvonnetangsu
Copy link
Member

We may need a page of all html elements to review this.

Let me know if you need anything from me.

@sherakama sherakama temporarily deployed to Tugboat February 9, 2020 00:13 Destroyed
@sherakama
Copy link
Member Author

I'm open to ideas. Do we want a public page with all the HTML elements we have styled for both testing for us and for the public?

@yvonnetangsu
Copy link
Member

I'm open to ideas. Do we want a public page with all the HTML elements we have styled for both testing for us and for the public?

Yeah, either that or we separate things into child pages, e.g., one for all form elements. Actually, perhaps we make a list of all the HTML elements we want to include then we could decide if it's ok to put them all on one page or if that gets too long?

Things I can think of:

  • Tables
  • Form stuff (text input, text area, radio/checkbox, select dropdown, submit buttons, whole form etc.)
  • lists - default ul, ol, and dl, also our custom styles, e.g., Kerri's list with lines in between, big number lists

Anything else?

@sherakama
Copy link
Member Author

Yeah, it might be a good idea to get them separated. We are often asked for form elements and should look into providing not only more styled options but components for them as well. Anyhow, my vote is separation.

@yvonnetangsu
Copy link
Member

Yeah, it might be a good idea to get them separated. We are often asked for form elements and should look into providing not only more styled options but components for them as well. Anyhow, my vote is separation.

Sounds good to me. I vote for separation too. I've been wanting to do/see some styling on our form elements also!

@sherakama
Copy link
Member Author

If you'd like to take first pass at this please do as I don't think I will get the time to spend on it I would like to this month.

@sherakama
Copy link
Member Author

@kerri-augenstein
Copy link

kerri-augenstein commented Feb 10, 2020

@yvonnetangsu @sherakama, the Adapt project has specific needs for FORM ELEMENTS! YAY! So we'll definitely be creating and executing them at the root of Decanter, and sub-theming off of those for SAA/OOD, within the next few months.

@yvonnetangsu
Copy link
Member

@yvonnetangsu @sherakama, the Adapt project has specific needs for FORM ELEMENTS! YAY! So we'll definitely be creating and executing them at the root of Decanter, and sub-theming off of those for SAA/OOD, within the next few months.

Cool - let me know if you have any designs for these, Kerri (both default and variants). I'd be happy to write myself a ticket, or @jenbreese and I could each take on some elements (just a suggestion and I totally know you guys might be super busy with D8).

@yvonnetangsu
Copy link
Member

@yvonnetangsu @sherakama, the Adapt project has specific needs for FORM ELEMENTS! YAY! So we'll definitely be creating and executing them at the root of Decanter, and sub-theming off of those for SAA/OOD, within the next few months.

Cool - let me know if you have any designs for these, Kerri (both default and variants). I'd be happy to write myself a ticket, or @jenbreese and I could each take on some elements (just a suggestion and I totally know you guys might be super busy with D8).

@kerri-augenstein @buttonwillowsix

@kerri-augenstein
Copy link

@yvonnetangsu, hahaha, designs... hahahaha. Will be working on 'em in the next few weeks and getting design feedback from y'all.

@sherakama sherakama temporarily deployed to Tugboat February 16, 2020 00:05 Destroyed
@sherakama sherakama temporarily deployed to Tugboat February 23, 2020 00:05 Destroyed
@sherakama sherakama temporarily deployed to Tugboat March 1, 2020 00:05 Destroyed
@sherakama sherakama temporarily deployed to Tugboat March 8, 2020 00:05 Destroyed
@sherakama sherakama temporarily deployed to Tugboat March 15, 2020 00:11 Destroyed
@sherakama sherakama temporarily deployed to Tugboat March 22, 2020 00:14 Destroyed
@sherakama sherakama temporarily deployed to Tugboat March 29, 2020 00:18 Destroyed
@sherakama sherakama temporarily deployed to Tugboat April 14, 2020 21:15 Destroyed
@sherakama
Copy link
Member Author

Putting up for review so it doesn't fall behind again.

@yvonnetangsu
Copy link
Member

Putting up for review so it doesn't fall behind again.

@sherakama Just double checking - the table element isn't actually on the tugboat preview right?

@jenbreese
Copy link
Contributor

@yvonnetangsu and @sherakama I couldn't find a table element either.

@sherakama
Copy link
Member Author

sherakama commented Apr 29, 2020

@yvonnetangsu @jenbreese

I totally thought we had an elements page somewhere. Guess that page is gone. An alternative to test this branch would be to pull it into your current work and quickly set up a table.

@sherakama
Copy link
Member Author

Here is a live URL of this code in action: https://amptesting.sites.stanford.edu/wysiwyg/test-tables-layout-full-width

@yvonnetangsu
Copy link
Member

@yvonnetangsu @jenbreese

I totally thought we had an elements page somewhere. Guess that page is gone. An alternative to test this branch would be to pull it into your current work and quickly set up a table.

Yeah, there's one on the previous version of the site, but not on this Netlify one.

Are we going with this design for the default table style? Big improvements over the current one (you fixed the double border issue). Just want to know if there's a design we're going for.

@kerri-augenstein
Copy link

@yvonnetangsu @jenbreese, yea, I think we should go with the design that's shown on the page Shea shared.

https://amptesting.sites.stanford.edu/wysiwyg/test-tables-layout-full-width

We tweaked some things that I think work well in the root of Decanter.

Copy link
Member

@yvonnetangsu yvonnetangsu left a comment

Choose a reason for hiding this comment

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

Another question - what do you think about removing the margin-bottom from the last element in each table cell, eg, a <p>? Since the table cell padding already take care of the spacing at the bottom. Other than that and the outside margin, looks great to me! 👍

@yvonnetangsu
Copy link
Member

Screen Shot 2020-04-30 at 9 35 48 AM
^ ^ ^ That's the margin-bottom I was referring to.

And have you seen this newer MARTY video btw? 😄
https://www.youtube.com/watch?v=3x3SqeSdrAE

@jenbreese
Copy link
Contributor

I like the idea of removing the margin-bottom. I think it would surprise users to have the extra space in a table.

@sherakama
Copy link
Member Author

@yvonnetangsu @jenbreese ready for review again.

@jenbreese
Copy link
Contributor

I think we want something more like this. But I don't think we want to use the * or to list out all the possibilities.

&:last-child {
        p {
          @include margin(null null 0 null);
        }
      }

As it is in now, I get this look. (Borders I added to make easier to see.)
Screen Shot 2020-10-06 at 9 06 37 AM

@sherakama sherakama temporarily deployed to Tugboat October 7, 2020 17:18 Destroyed
@github-actions github-actions bot added the size/m label Oct 7, 2020
@sherakama
Copy link
Member Author

Ok, I changed it to target the child elements. I think we have to use * here as it could really be anything inside that td/th.

Copy link
Member

@yvonnetangsu yvonnetangsu left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! Looks good to me for base table style. GTG

@jenbreese
Copy link
Contributor

@yvonnetangsu Want me to merge since you approved?

@yvonnetangsu
Copy link
Member

@yvonnetangsu Want me to merge since you approved?

Hi @jenbreese Sure! Was waiting to see if you have anything else to add 😄 Thanks!

@jenbreese jenbreese merged commit d8a102a into master Oct 7, 2020
@jenbreese jenbreese deleted the D8CORE-1261-table-styles branch October 7, 2020 20:26
@sherakama
Copy link
Member Author

Woot. 6 months of work. YAY!

@yvonnetangsu
Copy link
Member

Woot. 6 months of work. YAY!

You're doing better than me at un-staling 😂 🎉

yvonnetangsu added a commit that referenced this pull request Oct 8, 2020
* master:
  D8CORE-1261: Revised table styles. (#617)
  Create auto-label action (#763)
  Add stale action. (#762)
  D8CORE-1623: Adding a mixin and icons for a simple icon before dis… (#690)
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants