Problem/Motivation

After the child issues for #3328115: [META] Class/CSS cleanup were all completed, it was apparent that ProjectGrid was overlooked. In this issue, that component will be cleaned up + and we'll use this to provide other refactorings/cleanups that are easier to identify now that the majority of the CSS optimizations have been completed.

Steps to reproduce

Proposed resolution

Remaining tasks

  • ✅ File an issue about this project
  • ☐ Manual Testing
  • ☐ Code Review
  • ☐ Accessibility Review
  • ☐ Automated tests needed/written?

Issue fork project_browser-3333456

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

bnjmnm created an issue. See original summary.

chrisfromredfin made their first commit to this issue’s fork.

bnjmnm opened merge request !352

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Active » Needs review
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Issue summary: View changes

Since there are a bunch of miscellaneous refactorings, I left "authors notes" for most changes to make review easier.

Despite creating several new components, all of which have various import/export needs, this MR removes more lines of code than it adds, as it addresses several instances of repetitive code.

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented
Status: Needs review » Needs work

All the comments and code refactoring looks OK to me. I am not sure that you strictly need the selector before ::placeholder, as it's scoped, but I prefer having it nonetheless.

However, I rebased this and tested in DrupalPod and am seeing:
Error: Call to a member function getMaintenanceOptions() on bool in Drupal\project_browser\Controller\BrowserController->browse() (line 153 of /var/www/html/repos/project_browser/src/Controller/BrowserController.php).

Can anyone reproduce that, or is it just a DrupalPod issue?

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented

The message you're getting in #6 seems like it could happen if one of the configured sources didn't load correctly. It would be due to something on the PHP end, and the only PHP changes in this MR are to tests.

Since I'm the person that made most of the MR, I wouldn't take offense to wanting a second opinion before diagnosing that as unrelated. I think the process of ruling that out can be reviewing the files that are changed and confirming none would impact BrowserController , which does its job before the Svelte code is initiated.

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented
Status: Needs work » Reviewed & tested by the community

OK, can confirm that this is working on a clean local install. Must've been a DrupalPod issue.

fjgarlin made their first commit to this issue’s fork.

fjgarlin’s picture

fjgarlin commented

Rebasing to latest and doing a final round of testing.

fjgarlin’s picture

fjgarlin commented

All looking good, merging. Thanks!!
Failing CI, not sure why.

  • fjgarlin committed 3e746eed on 1.0.x authored by bnjmnm 
    Issue #3333456 by bnjmnm, chrisfromredfin, fjgarlin:  ProjectGrid CSS...
fjgarlin’s picture

fjgarlin commented
Status: Reviewed & tested by the community » Fixed

Thanks for the DrupalCI fix. All merged now.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.