-
Notifications
You must be signed in to change notification settings - Fork 38
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
[UX] Rename text format "Filtered HTML" label to "Basic" #1188
Comments
If we do this, I'd prefer we did it right and renamed the underlying format machine name as well, which will break a lot of tests (I'm guessing here). While it would be great to get this in before tomorrow, time is short and this could easily wait until 1.3.0. |
I would like to make this change in the Standard install profile, but not for people who have existing sites as that might be too confusing. Pushing an updated PR that changes both the label and the underlying machine name of the format. (edit: this is now my oldest feature branch!) |
I'm all for this 👍 ...the PR sandbox looks good, but there are still test failures. |
Rebased, and pushed a clean PR. |
@jenlampton - Sandbox needs refresh! |
It's not just a sandbox refresh, there are conflicts and possibly even failing tests. Back to "needs work". @stpaultim and @jenlampton I'm aware you're currently looking for "good first issues", but I wouldn't ever ever call anything regarding fixing tests or resolving conflicts easy. 😉 |
Tested backdrop/backdrop#4096. Applied the patch, installed a new Backdrop site. Went to edit one of the sample nodes: WFM. Code reviewed, LGTM. I will close the other PR backdrop/backdrop#2885, and will suggest that further discussion of changing the machine name (if needed) be in a separate issue. |
I do have concerns, as the active config for that filter on existing sites isn't updated (needs update hook). Is the label supposed to get updated? |
@jenlampton had said:
So no update hook or change on existing sites. |
@quicksketch asked for input on this issue today in the dev meeting. I'm in favor of this change, it's a follow-up to #4499, when we changed Full HTML to Raw HTML. I don't think "Filtered HTML" means much to most site editors for what is in fact the "Basic" default text editor in Backdrop. We have the experience of changing "Full HTML" to "Raw HTML" to mimic. That change went pretty seamlessly, in my opinion. This will ONLY effect new sites as it's part of the standard install profile. Have we looked at the Raw HTML PR to see if there is anything we are missing from this one? |
We did have this line in the previous PR to help document the change. It might be good to include the same here:
I'm a little surprised that this PR is so much shorter than the previous one: |
@stpaultim I've filed a fresh PR based off of the one Peter started. I rebased it, fixed PHPCS complaints (mostly lines exceeding 80 characters), and a few grammatical errors here and there in the relevant files/comments. This commit is the rebase (no actual changes, as there were no conflicts with the PR branch and 1.x): backdrop/backdrop@cda3d56 Can I please get a code review on this commit that has the additional changes over what @bugfolder previously tested/reviewed: backdrop/backdrop@730208c (not adding the
Yup, I've added a similar line in this PR as well. Thanks for pointing that out 👍🏼 |
Although I'm not totally sure about this, I think @stpaultim makes a good point that we've already done this once before. So I've merged backdrop/backdrop#4576 into 1.x for 1.27.0. I retitled this issue to indicate we only changed the label, not the underlying machine name. backdrop/backdrop@5fd0697 by @BWPanda, @klonos, @jenlampton, @indigoxela, @stpaultim, @philsward, and @bugfolder. Thanks folks! |
Let's remove the word "HTML" from the Filtered HTML format name. Since you can't enter HTML here anymore, having a format name that indicates that you can is confusing.
I'm proposing we change the name of this format to "Basic".
Related issue: [UX] Change the name of the "Full HTML" text format to "Raw HTML" - #4499 -
EDIT [This related issue is done and merged into core]
The text was updated successfully, but these errors were encountered: