[Olivero Theme] [Design QA]: Tighten up the Tables

Created on 2 November 2020, about 4 years ago
Updated 10 February 2023, almost 2 years ago

Problem/Motivation

This particular issue came out from the design QA review process conducted by @jwitkowski79.

There seems to be a bug in the table header row when you start to scroll down

  • Text in the table header becomes larger
  • Text in table header row becomes center aligned
  • Can we keep the styles for the table header row consistent so the same styles that you see before you start scrolling apply to the fixed header? See attached screenshots

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

📌 Task
Status

Needs work

Version

10.1

Component
Olivero 

Last updated 3 days ago

Created by

🇺🇸United States proeung Sewell, NJ

Live updates comments and jobs are added and updated live.
  • Needs issue summary update

    Issue summaries save everyone time if they are kept up-to-date. See Update issue summary task instructions.

  • Needs screenshots

    The change alters the user interface, so before and after screenshots should be added to document the UI change. Make sure to capture the relevant region only. Use a tool such as Aviary on Windows or Skitch on Mac OS X.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇮🇳India Shubham Sharma 77

    Applied patch #12 applied successfully in drupal-10.1.x-dev. We can move this ticket to RTBC.
    All the above-mentioned issues are fixed now.
    For ref sharing screenshots...

  • Status changed to Needs work almost 2 years ago
  • 🇺🇸United States bnjmnm Ann Arbor, MI
    +++ b/core/themes/olivero/css/components/table.css
    @@ -101,6 +101,18 @@
    +.sticky-header th {
    +  margin-block: 0;
    +  margin-inline-start: 0;
    +  margin-inline-end: 0;
    +  text-align: start;
    +  letter-spacing: 0.02em;
    +  color: var(--color-text-neutral-loud);
    +  font-family: var(--font-sans);
    +  font-size: 0.875rem;
    +  line-height: var(--sp);
    +}
    

    Adding the .views-table class to the element with .sticky-header eliminates the need for all this additional CSS. Aside from being less to change, this has the added benefit of ensuring any change to the not-sticky header styles will automatically be applied to the sticky header as well. With this approach, theres a risk of changing th styles in one place and neglecting to make the corresponding change in the other.

  • Status changed to Needs review almost 2 years ago
  • 🇮🇳India gauravvvv Delhi, India

    Addressed #14, Attached patch for same. Please review

  • 🇮🇳India _utsavsharma

    Fixed CCF for #15.

  • 🇮🇳India nayana_mvr

    Verified the patch #16 and tested it on Drupal version 10.1.x. The patch works fine and I have added the before and after screen recordings for reference.

  • Status changed to Needs work almost 2 years ago
  • 🇺🇸United States smustgrave

    This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

    Can the issue summary be updated with the proposed solution. I see the points of the issue but what was decided to solve them?

    Before/after screenshots of the issue would be useful as well.

  • 🇺🇸United States bnjmnm Ann Arbor, MI
    index 4c60e6d517..d939d8d2f9 100644
    --- a/core/misc/tableheader.js
    
    --- a/core/misc/tableheader.js
    +++ b/core/misc/tableheader.js
    
    +++ b/core/misc/tableheader.js
    @@ -211,7 +211,9 @@
    
    @@ -211,7 +211,9 @@
             // Clone the table header so it inherits original jQuery properties.
             const $stickyHeader = this.$originalHeader.clone(true);
             // Hide the table to avoid a flash of the header clone upon page load.
    -        this.$stickyTable = $('<table class="sticky-header"></table>')
    +        this.$stickyTable = $(
    +          '<table class="sticky-header views-table"></table>',
    +        )
               .css({
                 visibility: 'hidden',
                 position: 'fixed',
    

    This is a change to core code, so it would impact all themes. There would need to be testing with all core themes to confirm this doesn't introduce problems.

    Another option is to implement this in a manner specific to Olivero. One option is to override TableHeader.prototype.createSticky. The safest way is to probably invoke the original then add the class to this.$stickyTable. There are likely other options but that is one that comes to mind.

Production build 0.71.5 2024