search

silverstripe / silverstripe-asset-admin

Silverstripe assets gallery for asset management
BSD 3-Clause "New" or "Revised" License
19 stars 78 forks source link

TLN: change en.js word for "Details" to be "Edit" #1446

Closed sunnysideup closed 3 months ago

sunnysideup commented 3 months ago

Right now you see this when you select an image:

image

That does not make much sense, because you can already see the details and the button really is there to "EDIT" the image, as you can already see the details and because when you edit, you always edit "details"

image

For this purpose, I have added the english "phrase" for details now to be "edit".

michalkleiner commented 3 months ago

@sunnysideup is this addressing a particular issue? If there's one existing already, please link it, if not, please create one.

So far I can see there's https://github.com/silverstripe/silverstripe-asset-admin/issues/1209 and https://github.com/silverstripe/silverstripe-asset-admin/issues/1116 which both have relevant comments and info.

If we change the label, we probably also should change the translation key as we are changing the meaning so we need to let other languages know to re-translate it rather than just rely on the old meaning which may not be correct anymore.

sunnysideup commented 3 months ago

Will do.

sunnysideup commented 3 months ago

Putting my management hat on here for a second. If you look at the amount of discussion and the amount of time it has taken for this issue to be addressed then I think the ticket system is broken as a whole. Or perhaps the ticket creators like me are just too much in rush to do their due diligence / deliver a pull request properly.

In my books, it would be easier for someone who is "trusted" just to take up the issue, read the comments, make a decision, fix it (if required), and move on.

This sort of thing should be picked up by a more design conscious person and just addressed as such. It could simply be a pencil button rather than "edit" OR "details".

GuySartorelli commented 3 months ago

Sounds like there's a lot more to this than will be covered by this PR. I'm going to close this PR, but I'll also highlight this issue internally as something we should prioritise since it has caused so much confusion.

sunnysideup commented 3 months ago

@GuySartorelli - does it matter that AssetAdmin.DETAILS is not currently in the en dictionary?

GuySartorelli commented 3 months ago

Yes, that's a bug. Unfortunately the text collector doesn't currently collect text from JS so those have to be updated manually. Feel free to raise an issue for it, with a linked PR if you're up for it.

GuySartorelli commented 3 months ago

For clarity: You should be able to apply project-level override without it being added.