🇺🇸United States @smustgrave

Account created on 30 June 2015, over 10 years ago
  • Software Engineer at Mobomo 
#

Merge Requests

More

Recent comments

🇺🇸United States smustgrave

Thanks I’ll review it!

🇺🇸United States smustgrave

I like the replacement!

🇺🇸United States smustgrave

May be over engineering but should it be a configuration option for something in the settings.php that could be set?

🇺🇸United States smustgrave

The related issue is scary lol but believe this one is good to go. Much cleaner too :)

🇺🇸United States smustgrave

It’s fine to put back

🇺🇸United States smustgrave

Wanted to bump 1 more time if still valid? Still a major?

🇺🇸United States smustgrave

Wanted to bump 1 more time.

🇺🇸United States smustgrave

Works for me!

🇺🇸United States smustgrave

since there's been no follow up and this kinda felt like a feature request vs a task going to close out. If I'm wrong please re-open.

🇺🇸United States smustgrave

Method still appears to return object or false so probably still valid.

🇺🇸United States smustgrave

since there's been no follow up and as a feature request going to close out. Can always be re-opened

Also know we are phasing out jquery best we can.

🇺🇸United States smustgrave

@xjm so still valid?

🇺🇸United States smustgrave

May still be valid.

🇺🇸United States smustgrave

Since there's been no follow up going to close out. If still an issue in D11 please re-open!

🇺🇸United States smustgrave

Since there's been no follow up for steps going to close out. If still an issue in D11 please re-open updating summary with steps.

Thanks all!

🇺🇸United States smustgrave

@kovalevm did you check your theme as mentioned in #4?

🇺🇸United States smustgrave

Wanted to follow up again.

🇺🇸United States smustgrave

Thank you for sharing your idea for improving Drupal.

We are working to decide if this proposal meets the Criteria for evaluating proposed changes. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or there is no community support. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

🇺🇸United States smustgrave

Thank you for sharing your idea for improving Drupal.

We are working to decide if this proposal meets the Criteria for evaluating proposed changes. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or there is no community support. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

🇺🇸United States smustgrave

Thank you for reporting this problem. We rely on issue reports like this one to resolve bugs and improve Drupal core.

Since there has been no activity here for over 8 years we are asking if this problem persists on a currently supported version of Drupal. To help, add a comment explaining if the problem still occurs or not. Any extra detail you can provide can help others who experienced this.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.
Thanks!

🇺🇸United States smustgrave

Title and MR don't line up exactly.

🇺🇸United States smustgrave

Was the original goal to give a token example? Is there a different token that could be used?

🇺🇸United States smustgrave

Possible to get a small test covering this change?

🇺🇸United States smustgrave

MR appears to have pipeline issues

With regards to the post_update hook this something may want to use a ConfigImporter class to run in batches?

Post update hook will need test coverage too

Thanks!

🇺🇸United States smustgrave

So I'm sure these fluctuate but compared this run to the last run from HEAD

Javascript #1 3 min 27 sec compared to 3 min 41 sec
Javascript #2 3 min 7 sec compared to 5 min 12 sec
Javascript #3 3 min 34 sec compared to 2 min 53 sec

So definitely improvement in 2/3 of the runs. The #3 may have been an outlier since there's such a difference in #2

Going to mark as a net improvement.

🇺🇸United States smustgrave

So did some manual testing.

On a fresh 11.x install
Standard profile install
Created a few Articles and Basic page content
Ran php core/scripts/drupal content:export node --dir=my-content --bundle=page
Verified ONLY Basic pages were exported
Did the same for Articles

Appears to be working as expected!

Tagging for a CR though.

Leaving in review for others

🇺🇸United States smustgrave

That would work too +1

🇺🇸United States smustgrave

re-reading #36 also mentions a release note

If no one else has a concern #39 it can be ignored.

🇺🇸United States smustgrave

smustgrave created an issue.

🇺🇸United States smustgrave

I found the issue this was done in Use modal in add new field flow Active as I was surprised this approach of a try/catch was accepted but that was a pretty significant change so maybe just lost in the shuffle.

Seems like a good refactor

I did have to look up where 'General' came from but that was easy.

🇺🇸United States smustgrave

Thanks feedback appears to be addressed

🇺🇸United States smustgrave

Left some comments on the MR. Thanks!

🇺🇸United States smustgrave

Seems this may need a manual rebase.

🇺🇸United States smustgrave

Now that we have the Development settings page that isn't stored in config we could extend that form to set

$settings['extension_discovery_scan_tests'] too ?

🇺🇸United States smustgrave

Believe the merge conflict is valid now in core/.deprecation-ignore.txt. Bot occasionally has it's moments.

🇺🇸United States smustgrave

Ran the test-only run here https://git.drupalcode.org/issue/drupal-3532360/-/jobs/6995154 to show the coverage

Left a comment on the MR.

If you are another contributor eager to jump in, please allow the previous poster(s) at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

🇺🇸United States smustgrave

With or without the MR I'm getting

Something maybe changed on the ckeditor5 side

🇺🇸United States smustgrave

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

🇺🇸United States smustgrave

Believe still valid.

🇺🇸United States smustgrave

Believe still valid.

🇺🇸United States smustgrave

Wanted to bump 1 more time

🇺🇸United States smustgrave

Wanted to bump 1 more time before closing;

🇺🇸United States smustgrave

Still seems relevant

Tagging for an IS update if the idea to deprecate 1.

🇺🇸United States smustgrave

Since there's not been a follow up and as a feature request going to close out. Can always be re-opened

Thanks all!

🇺🇸United States smustgrave

Think need to re-look at this. All the test changes seem like this would be an immediate breaking change for contrib and custom tests.

Example
$page->fillField('label', 'Bears again');

Is very common so we should still be able to handle this

🇺🇸United States smustgrave

Think we should be logging an error if the role doesn't exist.

🇺🇸United States smustgrave

Resolved what threads I could. But don't want to bypass @xjm review. xjm do you mind checking your open threads please?

🇺🇸United States smustgrave

Not to be used by core but uploading a txt file of the mjs file I used on a contrib theme using eslint-config-airbnb-extended

ChatGPT did help some.

🇺🇸United States smustgrave

All I did was rebase but appears some upstream changes broke the unit tests. Left a small comment that should address them.

🇺🇸United States smustgrave

I meant LoggerChannelTrait;. Then just need to call $this->getLogger()->warning()

🇺🇸United States smustgrave

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

🇺🇸United States smustgrave

CR reads fine to me. Can't wait to add this to some of my ui_suite_uswds components. I didn't mark fixed as didn't want to jump the gun.

🇺🇸United States smustgrave

So not 100% sure how to review this one

🇺🇸United States smustgrave

Left a small comment but leaving in review for others.

🇺🇸United States smustgrave
🇺🇸United States smustgrave

This was provided to me in slack https://webaim.org/techniques/keyboard/

Ensure elements with ARIA role="button" can be activated with both key commands.

I can confirm that tabbing through the toolbar space now opens the manage section.

Will say from a user standpoint the previous behavior of using space scrolled the page down and I thought that was better. But acknowledge the accessibility rule.

Moving to NW for the change record.

🇺🇸United States smustgrave

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

🇺🇸United States smustgrave

Lets update the IS to clear up #36 please.

🇺🇸United States smustgrave

IS still appears incomplete needs to use the issue template.

Not reviewed.

🇺🇸United States smustgrave

Revert the previous test but ran the test-only job https://git.drupalcode.org/issue/drupal-3553853/-/jobs/6994061

🇺🇸United States smustgrave

So I actually got the wrong test but took the feedback from that one to the correct one.

🇺🇸United States smustgrave

Original concern of updating the the twig templates directly is back. We will need a CR as contrib and custom themes won't have that change.

🇺🇸United States smustgrave

I'm told he already did so ignore #7 :)

🇺🇸United States smustgrave

In that case I would be a +1 but would be more comfortable is maybe @catch could give it a look. I'm happy to mark though so he can still merge.

🇺🇸United States smustgrave

Maybe we could add a simple warning or message if content for the targeted node already exists?

🇺🇸United States smustgrave

This came up as a daily BSI target.

This seems like a task vs a bug but agree maybe something can be done?

🇺🇸United States smustgrave

Wanted to bump 1 more time for steps else we could close.

🇺🇸United States smustgrave

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

🇺🇸United States smustgrave

Was going to tag stale-issue-content but this seems like it's probably still relevant.

🇺🇸United States smustgrave

Thank you for reporting this problem. We rely on issue reports like this one to resolve bugs and improve Drupal core.

Since there has been no activity here for over 8 years we are asking if this problem persists on a currently supported version of Drupal. To help, add a comment explaining if the problem still occurs or not. Any extra detail you can provide can help others who experienced this.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.
Thanks!

🇺🇸United States smustgrave

I could for sure! I get a few days a month from work for contribution work.

As mentioned probably would close out the D7 stuff because that's done.

For the D10+ stuff I see you have amazing test coverage so would continue on that path for all fixes or new features.

🇺🇸United States smustgrave

Will request tests for this too please.

🇺🇸United States smustgrave

Cleaned up the IS some but also dove into the history and that was quit a hole lol. Agree I can't find a reason why it was cutoff. Seems was added as part of a security release and since @prudloff is on the security team and doesn't see a reason to cutoff think this fix is fine.

🇺🇸United States smustgrave

Outputting in the steps

Nightwatch
cSpell
Eslint
Stylelint

Makes sense to me.

Verified it works

🇺🇸United States smustgrave

Just going to post questions

1. I'm seeing the deprecations are changing in WorkspaceManager but believe these changes have already been in a release so is that okay?
2. I see we are switching to cron and limiting to 50, which makes sense. My question is if those workspaces are large with lots of content each any concern using cron then?
3. Do we need a CR for the new Event and Subscriber?

Manual tests I did

Base setup.
1. Latest 11.x
2. Standard profile install
3. Apply MR
4. cleared cache

Managing spaces
I created a new workspace (dev)
Switched to it
Switched to stage
Deleted dev

Working

Publishing content

Switched to stage
Created an Article (Test stage)
Switched to live
Made sure I could not edit Test stage
Switched to stage
Published content
Seeing the Test stage page in live space

Working

Functionality wise appears to be working.

🇺🇸United States smustgrave

I don't want to derail this at all. Is there functionality from linkit that won't be covered by this change? Just to help evaluate using this later.

🇺🇸United States smustgrave

These look correct to me. Thanks!

🇺🇸United States smustgrave

Believe what @catch was referring to was this needs a post_update hook. The schema entry nullable is saying the value can be null (which may not be correct) but the key is still missing from existing views. Here's a scenario

Do a config export of a view
Apply this MR
Save the view again making 0 changes
Export the view again and notice it has changed because this key was added but I didn't do anything.

So this needs a post_update that uses a configImporter to help. This way it's ran in batches because some sites could have 100s of views. An example would be views_post_update_table_css_class which calls a function in the viewConfigImporter

Then the test coverage for that would be something maybe like testConfigUpdate().
1. Check view doesn't have this new key
2. Run updates
3. Check view does have this new key

Hope this helps.

🇺🇸United States smustgrave

Before

After

I also checked Require email verification when a visitor creates an account which should have aria-described

Ran the test-only job here https://git.drupalcode.org/issue/drupal-2839344/-/jobs/6981702 which shows the coverage

Believe this one is good to go.

🇺🇸United States smustgrave

Hope that helps!

🇺🇸United States smustgrave
🇺🇸United States smustgrave

Since there's been no follow up to how this is an accessibility issue I'm going to close out. If still valid please re-open and update summary with how this is an accessibility issue please

Thanks all!

🇺🇸United States smustgrave

Since there's been no follow up and as mentioned there is a contrib module. Slimmer core stronger core :)

🇺🇸United States smustgrave

Wanted to follow up if you tried #3?

🇺🇸United States smustgrave
🇺🇸United States smustgrave

smustgrave created an issue.

🇺🇸United States smustgrave

Actually don't want to ping pong this I'll make a new ticket.

🇺🇸United States smustgrave

Will take a deeper look

🇺🇸United States smustgrave

Responded but that change is needed to fix a ckeditor multi line test. Seems like an oversight as the text with summary placeholder was type text

🇺🇸United States smustgrave

Reverted back

🇺🇸United States smustgrave

Hoping to do a release end of the week so if we can get test coverage we can include this.

🇺🇸United States smustgrave

Just wanted to see if you've had any luck (if using on a project) @macsim

🇺🇸United States smustgrave

Lets postpone till a fix is found for 🐛 Datepicker doesn't register relative dates Active

🇺🇸United States smustgrave

Will need a reroll for 7.1.x

🇺🇸United States smustgrave

Will include in 7.1.x as does seem like a net gain improvement.

Production build 0.71.5 2024