Problem/Motivation

They are structurally very similar, the main differences are added information depending on the value of toggleView. This has me wondering if it's possible to just have hidden attributes based on the value of toggleView. A little bit of conditional logic added that would make it possible to remove an entire component.

Aside from less code to maintain, this consolidation would eliminate the risk of of the two view drifting out of sync.

Steps to reproduce

Proposed resolution

Remaining tasks

  • ✅ File an issue about this project
  • ☐ Manual Testing
  • ☐ Code Review
  • ☐ Accessibility Review
  • ☐ Automated tests needed/written?
CommentFileSizeAuthor
#6 Screen Shot 2022-06-09 at 1.16.13 PM.png95.61 KBbnjmnm

Issue fork project_browser-3284325

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.

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

narendrar’s picture

narendrar
He/Him
Primary language English
Location Jaipur, India
commented
Assigned: Unassigned » narendrar

narendraR opened merge request !152

narendrar’s picture

narendrar
He/Him
Primary language English
Location Jaipur, India
commented
Assigned: narendrar » Unassigned
Status: Active » Needs review
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Needs review » Needs work
StatusFileSize
new Screen Shot 2022-06-09 at 1.16.13 PM.png95.61 KB

This looks great. I spotted one tiny style difference with the icons padding that should probably get addressed.

I'll go over the MR just to be sure, but this it looking like a welcome simplification.

narendrar’s picture

narendrar
He/Him
Primary language English
Location Jaipur, India
commented
Status: Needs work » Needs review
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Needs review » Reviewed & tested by the community

Looks good!

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Reviewed & tested by the community » Needs work

Setting to NW to surface the error spotted in the MR by tim.

narendrar’s picture

narendrar
He/Him
Primary language English
Location Jaipur, India
commented
Status: Needs work » Needs review
srishtiiee’s picture

srishtiiee commented
Status: Needs review » Reviewed & tested by the community

Looks good.

tim.plunkett made their first commit to this issue’s fork.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Reviewed & tested by the community » Fixed

This is a great refactor, thanks!

Status: Fixed » Closed (fixed)

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