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

task/tup-534 Adds in media query for tables #275

Merged
merged 32 commits into from
Jan 25, 2024
Merged

task/tup-534 Adds in media query for tables #275

merged 32 commits into from
Jan 25, 2024

Conversation

R-Tomas-Gonzalez
Copy link
Collaborator

@R-Tomas-Gonzalez R-Tomas-Gonzalez commented Nov 28, 2023

Overview

Added in mobile view for tables. Columns now go from horizontal to vertical in mobile view.
Replaces the old solution (in Core-Styles) (and any code mentioning APCD like it does) with the new solution (from APCD).

Related

Changes

(Most changes were done in table--basic. Does this need to happen elsewhere? @wesleyboar)

  • Added a media query to handle the table being on a smaller screen
  • Removed and added styles that pertain to different tables (Header, No Header, Paragraphs, Paragraphs with Headers, Nested Tables)

Testing

  1. Check for borders in correct spaces
  2. Check that all data is available and able to be viewed
  3. Watch for unwanted side-effects (missing data, unwanted padding, unwanted margin, borders in wrong spots)
  4. This is the start to a mobile solution that WILL be iterated on, doesn't have to be perfect (yet), but should have all of the ingredients.

UI

Web Mobile
Screenshot 2024-01-25 at 11 41 59 AM Screenshot 2024-01-25 at 11 42 08 AM
Screenshot 2024-01-25 at 11 43 43 AM Screenshot 2024-01-25 at 11 43 51 AM
Screenshot 2024-01-25 at 11 44 45 AM Screenshot 2024-01-25 at 11 44 58 AM
Screenshot 2024-01-25 at 11 46 24 AM Screenshot 2024-01-25 at 11 46 34 AM
Screenshot 2024-01-25 at 11 47 16 AM Screenshot 2024-01-25 at 11 47 29 AM

Specifically not working for nested tables just yet. Also, need to look at paragraph tables with no headers - padding adjusts for headers even when there are none...
Copy link
Member

@wesleyboar wesleyboar left a comment

Choose a reason for hiding this comment

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

Pre-review review 😅.

src/lib/_imports/elements/table--basic.css Outdated Show resolved Hide resolved
src/lib/_imports/elements/html-elements.cms.css Outdated Show resolved Hide resolved
src/lib/_imports/elements/table--basic.css Show resolved Hide resolved
src/lib/_imports/core-styles.docs.css Outdated Show resolved Hide resolved
wesleyboar and others added 5 commits November 30, 2023 11:48
* test: get correct data-col value (INCOMPLETE)

* feat: get correct col names & values (INCOMPLETE)

* feat: get correct col names & values (SUCCESS)

* chore: clean up new helper getColFromRow

* refactor: less chars in hbs, more logic in config

* fix: remove cruft change

* chore: remove console.log

* fix: missing cols breaks getColFromRow

* chore: simplify previous fix
@R-Tomas-Gonzalez R-Tomas-Gonzalez marked this pull request as ready for review December 1, 2023 18:48
@R-Tomas-Gonzalez R-Tomas-Gonzalez self-assigned this Dec 1, 2023
@R-Tomas-Gonzalez R-Tomas-Gonzalez added enhancement Improvements or additions to existing features patch A backward-compatible fix minor A feature in backward-compatible manner labels Dec 1, 2023
accomplishes 1 - 3 on design dev mtg notes.(will attach comments).
@R-Tomas-Gonzalez
Copy link
Collaborator Author

R-Tomas-Gonzalez commented Dec 7, 2023

b440a77

Makes these adjustments:

  1. IMPORTANT:
  • On all sizes, increase spacing e.g. --cell-pad-vert, --cell-pad-horz, line-spacing.
  • On "mobile design", increase spacing even more.
  1. IMPORTANT: On "mobile design" and smaller, use larger font.
  • Done?
  1. DOUBLE CHECK: Verify that top border and bottom border on tables mobile and desktop match their status on main branch (dev.tup) and are consistent.
  • If borders are inconsistent on task/tup-534 branch, that might be a new bug. Check main.
  • If borders are inconsistent on main branch, that's a main branch bug you may fix in task/tup-534.
  1. NICE TO HAVE: On "mobile design" (on desktop), can hovering over column name create a tooltip (without JavaScript). I.e. use title attribute as tooltip.
    • Wes thinks, "Probably not."

Tomas: Did not try this. Will worry about "Nice To Haves" when we have working code.

@R-Tomas-Gonzalez
Copy link
Collaborator Author

396b40d

Makes the following change. Having portal not truncate was decided here

  1. REMEMBER: Paragraphs should not be truncated by default. Portal may truncate them, though (Tomas: No longer the case due to commented out code).
  2. Brings back the top border for paragraphs

 we can adjust from here when/if needed. But for now, they are mimiced from desktop to mobile
Awaiting design for feedback on nested tables.
Copy link
Member

@wesleyboar wesleyboar left a comment

Choose a reason for hiding this comment

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

The result looks excellent — just as we expect while we wait for more design instruction. But I know the code to get there is a work in progress. I've left some notes to track the cleanup that can be done.

fractal.config.js Outdated Show resolved Hide resolved
src/lib/_imports/elements/table--nested.css Show resolved Hide resolved
src/lib/_imports/elements/table.cms.css Outdated Show resolved Hide resolved
src/lib/_imports/elements/table--basic.css Outdated Show resolved Hide resolved
src/lib/_imports/elements/table--basic.css Outdated Show resolved Hide resolved
src/lib/_imports/elements/table--basic.css Outdated Show resolved Hide resolved
src/lib/_imports/elements/table--basic.css Show resolved Hide resolved
src/lib/_imports/elements/table--basic.css Outdated Show resolved Hide resolved
src/lib/_imports/elements/table--nested.css Outdated Show resolved Hide resolved
@R-Tomas-Gonzalez R-Tomas-Gonzalez marked this pull request as ready for review January 18, 2024 19:13
peer-programmed with T.G. to confirm

Note: one style W.B. thought should go was retained because it does soemthing:

  table.has-table {
    & tr :is(td,th):last-child {
      border-bottom: unset;
    }
  }
Copy link
Member

@wesleyboar wesleyboar left a comment

Choose a reason for hiding this comment

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

For approval, please:

Copy link
Member

@wesleyboar wesleyboar left a comment

Choose a reason for hiding this comment

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

Found two bugs.

If, before fixing these, you already have screenshots updated since my last review (today), no need to update again.

src/lib/_imports/elements/table--basic.css Outdated Show resolved Hide resolved
src/lib/_imports/elements/table--basic.css Outdated Show resolved Hide resolved
Copy link
Member

@wesleyboar wesleyboar left a comment

Choose a reason for hiding this comment

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

Third bug found. Not a blocker. Double borders on mobile tables. I have trouble debugging it. I hope to avoid border removal bandaid solution.

Basic Table Paragraph Table
basic paragraph
Details

In table--basic.icss, there is a —

/* To add bottom border to rows */
tr:not(:last-of-type) > :is(td, th),
:--s-paragraph-table p:not(:last-of-type) {
  border-bottom-width: var(--global-border-width--normal);
  border-bottom-style: solid;
  border-bottom-color: var(--global-color-primary--light);
}

— and a —

@media (max-width: 767px) {
  ...

  tr > :is(td, th):not(:last-of-type),
  :--s-paragraph-table p:not(:last-of-type) {
    border-bottom-width: var(--global-border-width--normal);
    border-bottom-style: solid;
    border-bottom-color: var(--global-color-primary--light);
  }

  ...
}

Nested tables avoids double borders by selectively removing them. But maybe there is a solution (a set of selectors, and maybe a use of media queries), for basic and paragraph and nested tables, with less code that only applies the borders as needed, so they need not be removed after the fact.

@R-Tomas-Gonzalez
Copy link
Collaborator Author

R-Tomas-Gonzalez commented Jan 25, 2024

Third dug found. Not a blocker. Double borders on mobile tables. I have trouble debugging it. I hope to avoid border removal bandaid solution.

Basic Table Paragraph Table
basic paragraph

Let me know if this is a good resolve:
(Removes the unset in nested--table, and brings it to the scope of the basic--table)

8fc5c29

@wesleyboar
Copy link
Member

wesleyboar commented Jan 25, 2024

Let me know if this is a good resolve: (Removes the unset in nested--table, and brings it to the scope of the basic--table) 8fc5c29

It is good.

@wesleyboar wesleyboar removed the patch A backward-compatible fix label Jan 25, 2024
@wesleyboar wesleyboar reopened this Jan 25, 2024
@wesleyboar
Copy link
Member

GitHub trying to be smart by closing this PR for me cuz I said "fixed #275" in another PR… Do only what I say, GitHub.

Copy link
Member

@wesleyboar wesleyboar left a comment

Choose a reason for hiding this comment

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

YES!

@wesleyboar wesleyboar merged commit aeb1b32 into main Jan 25, 2024
@wesleyboar wesleyboar deleted the task/tup-534 branch January 25, 2024 20:27
wesleyboar added a commit that referenced this pull request Mar 1, 2024
wesleyboar added a commit that referenced this pull request Mar 1, 2024
* Revert "task/tup-534 Adds in media query for tables (#275)"

This reverts commit aeb1b32.

* more changes from revert n' build
@wesleyboar
Copy link
Member

Reverted in #313.
Restored in #317.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements or additions to existing features minor A feature in backward-compatible manner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants