search

phenaproxima / starshot-prototype

Prototype of a new kind of Drupal, based on recipes and loaded up with contrib's best modules and themes. Not a fork or a distribution.
https://drupal.org/starshot
115 stars 46 forks source link

Add the Type Tray module #77

Closed e0ipso closed 4 months ago

e0ipso commented 4 months ago

We have been successfully using Type Tray in all of our projects.

The reasoning behind using it is documented here.

image

phenaproxima commented 4 months ago

Oh, one other request, @e0ipso -- in another PR, can you opt the Basic block type into this as well? It will necessitate creating a new starshot_basic_block_type recipe that either builds on top of, or outright forks, core's basic_block_type.

pameeela commented 4 months ago

I was initially on the fence about this, but I tried it out and it is an improvement. I wasn't sure about needing an icon for each content type, but I see there is a default icon that is perfectly fine. I would also just say the basic page icon proposed here doesn't really make sense to me, but it's not super important.

I also think the styling could use some love but that's what's great about Starshot, it should drive improvement in contrib as well :)

phenaproxima commented 4 months ago

the basic page icon proposed here doesn't really make sense to me

I agree on this. But that's very easily fixed later.

I think my only remaining concern here is the possibility of this breaking if the site directory is not sites/default. If we address that, ship it!

pameeela commented 4 months ago

public:// doesn't seem to work, based on testing. @phenaproxima you initially said the file location could be a follow up, do you still think so? Your last comment seems to imply we should solve it first.

phenaproxima commented 4 months ago

I think it can be a follow-up as long as we have an issue open -- ideally in Type Tray's issue queue, with a follow-up issue here to use the fix when there's a patch. @e0ipso, if you open those things, we can merge this.

phenaproxima commented 4 months ago

Opened an issue for this, with a patch (no tests yet): https://www.drupal.org/project/type_tray/issues/3447694

Can we apply that patch here, and switch to using stream wrappers?

e0ipso commented 4 months ago

Sorry for taking this long to reply. I will be looking into this today.

e0ipso commented 4 months ago

@phenaproxima @pameeela this was fixed. Let me know if there is anything else.

phenaproxima commented 4 months ago

Thanks @e0ipso et. al., this looks great.

pameeela commented 4 months ago

I'm wondering about adding this now in light of a few other ongoing discussions about how to vet modules. This one is (pretty much) purely a visual change, not really functional. It does offer categorisation to content types, but that is unlikely to be useful for the Starshot audience I think, because it would only be needed if there are a lot of content types.

I still really like the module but now I'm second guessing the addition!

phenaproxima commented 4 months ago

It's true that Type Tray's changes are visual, but IMHO they are visual in a way that increases Drupal's user-friendliness and usability, particularly by supporting type-specific icons. That's why I added it.

I think it's completely fine and in line with Starshot's mission to add "cosmetic" modules if they enhance usability. That's why I'm on the fence about Gin Login, which absolutely is a visual upgrade, but not necessarily a usability upgrade.

gitressa commented 4 months ago

Which modules to add, and which not to add ... it's a fine balance. There are nice-to-have modules, and then there are modules which add a lot of value. Every "nice" module included risk adding technical debt, and -- by definition -- will increase the number of modules, at the risk of ballooning into a sea of modules, if not kept in check. Keeping the number of modules at a reasonable level is good practice, in my opinion.