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

Put action name in heading #4902

Closed
wants to merge 1 commit into from

Conversation

pekka
Copy link
Contributor

@pekka pekka commented Jan 19, 2023

WHY

See #4901

BEFORE - What was wrong? What was happening before this PR?

The "create" and "edit" pages had their headings set to the model plural (e.g. "Monsters") and the subheading to the current action (e.g. "create new monster").

image

AFTER - What is happening after this PR?

  • The heading will be set to the current action (unless something is set in $crud->getHeading())
  • The subheading will be empty by default, unless something is set in $crud->getSubheading()

HOW

Switched the placeholders in the template code

Is it a breaking change?

No

How can we test the before & after?

Create or edit any model

@scrutinizer-notifier
Copy link

The inspection completed: No new issues

@pxpm
Copy link
Contributor

pxpm commented Jan 19, 2023

Hello @pekka thanks for the PR and the explanation 🙏

I agree with some of your arguments:

  • It's better to read Edit Monsters than Monsters Edit Monsters for screen readers.
  • Monsters Edit Monsters is redundant, indeed.

What I don't like is that Edit word in this PR has the same emphasis as the "heading" of the page.

To fix the above mentioned issues should we just get rid of the monsters word in the subheading, making it only:
Monsters Edit ?

Would that still be an issue for screen readers ? Redundancy is removed so maybe that works ?

@tabacitu what do you think ?

Cheers

@pekka
Copy link
Contributor Author

pekka commented Jan 19, 2023

To fix the above mentioned issues should we just get rid of the monsters word in the subheading, making it only: Monsters Edit ?
Would that still be an issue for screen readers ? Redundancy is removed so maybe that works ?

Personally, I think describing the current operation in plain text would work best! But, your decision, and trivially easy to customize anyway 👍
I can amend the PR once it's decided.

@pxpm
Copy link
Contributor

pxpm commented Jul 17, 2024

Hey @pekka 10 years later ... 🫨

This PR as is I think will not get merged, but I like the idea myself.

I thought about a solution for this here: #5566 not sure if it's a good idea or not. If you have an opinion about it, let me know 🤷‍♂️

Thanks again for taking time to submit the PR 🙏

@pxpm pxpm closed this Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants