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

WP-193: core-stlyes update #837

Merged
merged 3 commits into from
Aug 2, 2023
Merged

Conversation

van-go
Copy link
Contributor

@van-go van-go commented Jul 19, 2023

Overview

Upgrade the core-styles package in package.json

Related

Changes

Ran "npm install @tacc/core-styles" to update core-styles, this made changes to client/package-lock.json and client/package.json

Testing

  1. Ran "npm list" to verify the updated version. This returned "@tacc/[email protected]"
  2. Ran "npm audit" to check for vulnerabilities. This returned:
    postcss <7.0.36
    Severity: moderate
    Regular Expression Denial of Service in postcss - GHSA-566m-qj78-rww5
    No fix available
    node_modules/postcss-extend/node_modules/postcss
    postcss-extend *
    Depends on vulnerable versions of postcss
    node_modules/postcss-extend
    @tacc/core-styles *
    Depends on vulnerable versions of postcss-extend
    node_modules/@tacc/core-styles
    Ran "npm list postcss". This returned "[email protected]" which indicates there should not be an issue.
  3. Checked for changes in font, display, etc. Compared the following pages locally to prod:
    Dashboard
    —> My Recent Jobs
    —> System Status (DOES THIS EXIST IN PROD)
    —> My Tickets
    —> Open Ticket
    —> New Ticket
    —> Manage Account
    Data Files
    —> My Data (Corral, Work, Frontera)
    —> Community Data
    —> Public Data
    —> Shared Workspaces (Owner is missing locally)
    —> Google Drive
    —> Search Bar
    —> Filter
    —> Toolbar (buttons)
    Applications
    —> Applications (Visualization)
    Allocations
    —> Request new Allocation
    —> View Team
    —> View User Info
    —> Expired
    —> View Team
    —> View User Info
    History
    —> Jobs
    —> Search
    —> Pre-June 2023 (Called Historic Jobs in testing)
  4. Resized Dashboard to review its three or four supported layouts. Did not notice any differences.

UI

Dashboard
Screen Shot 2023-07-19 at 11 55 00 AM

Data Files
Screen Shot 2023-07-19 at 11 56 07 AM

Applications
Screen Shot 2023-07-19 at 11 57 10 AM

Allocations
Screen Shot 2023-07-19 at 11 58 00 AM

History
Screen Shot 2023-07-19 at 11 58 44 AM

Dashboard Re-sized
Screen Shot 2023-07-19 at 12 15 02 PM

Notes

@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

Merging #837 (ab4b975) into main (39cc145) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #837   +/-   ##
=======================================
  Coverage   63.10%   63.10%           
=======================================
  Files         425      425           
  Lines       12095    12095           
  Branches     2465     2465           
=======================================
  Hits         7633     7633           
  Misses       4262     4262           
  Partials      200      200           
Flag Coverage Δ
javascript 68.95% <ø> (ø)
unittests 57.30% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Contributor

@edmondsgarrett edmondsgarrett left a comment

Choose a reason for hiding this comment

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

Most everything is looking normal, nice! It seems like (at least some) app icons are missing though, could be due to name change for some of the icons?

@edmondsgarrett edmondsgarrett self-requested a review July 26, 2023 16:13
Copy link
Contributor

@edmondsgarrett edmondsgarrett left a comment

Choose a reason for hiding this comment

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

LGTM! Icons missing doesn't appear to be related to this PR, also happening on main.

@rstijerina
Copy link
Member

LGTM! Icons missing doesn't appear to be related to this PR, also happening on main.

FYI you should be able to address missing icons for individual apps here: https://cep.test/core/admin/workspace/apptrayentry/

There are some old vestigial entries that may have non-existent icons. The supported icons are listed throughout client/src/styles/trumps/icon.fonts.css

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.

None of my fears were realized, because Core Portal still relies as minimally as possible on Core Styles.

Fears, and why they were unrealized…

The following are not affected by this change:

  • font sizes, because portal does not use --global-font-size--… yet
  • colors, because portal does not use --global-color-accent--xx… yet and still sets its own accent colors
  • space, because portal does not use --global-space-- much and still sets its own values for what it does use

@rstijerina rstijerina merged commit f8eacfd into main Aug 2, 2023
@rstijerina rstijerina deleted the task/wp-1930core-styles-update branch August 2, 2023 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants