surabhi-gokte → credited pooja_sharma → .
penyaskito → credited pooja_sharma → .
In #41 patch not working for existing field, so fixed the issue by adding missing required code for existing field & uploaded the patch
@larowlan, Thanks for reviewing , working on it
This issue is not replicate at my end, it 'll be helpful if more detail steps shared along with screencast.
Added the documentation ,go through the description. apart from it nothing seems to be left.
Please review, moving NR.
pooja_sharma → made their first commit to this issue’s fork.
pooja_sharma → created an issue.
This issue is fixed already in latest release 8.x-3.5 so closing it
pooja_sharma → created an issue.
Tour btn on navigation placed on the footer region on install module & reused existing tour btn & removed the unused code.
PLease review the latest changes
Addressed #28 📌 Fix Content Moderation tests that rely on UID1's super user behavior Active feedback, sorry it was missed from my end, now code looks clean & tried to compare the old & changed generated test html files output - both are same.
Please review, moving NR.
Thanks for reviewing it, Verifying to address #28 📌 Fix Content Moderation tests that rely on UID1's super user behavior Active feedback.
Reused same helper function to place the tour block with navigation so it's not possible that tour placement change, until we have manually move the tour block or either any other module block (like shortcut),
Tried to tackle here navigation & tour integration , if we try to test with shortcut (other module by uninstall/install ) then it can be possible there may be such scenario, it seems out of scope, we can ignore it.
Resolved MR conflicts, functionality seems working fine.
Please review , moving NR.
Tried to address feedback also covered following scenarios , please review , moving NR.
if tour module already installed, after navigation module install then "tour block" should place to the navigation sidebar: Done
if navigation module already installed, after tour module install then "tour block" should place to the navigation sidebar: Done
if tour & navigation both already installed , then update hook already added, that place "tour block" to the navigation sidebar: Done
By default place "tour block" place in navigation should be able to perform all crud operation: move , delete, configure (title update): Done
/admin/config/user-interface/navigation-block from here, user should able to place tour block from UI via click "Add content block".: Done
Resolving conflicts, verifying functionality
I have fixed as part of this MR, 'll check it & 'll resolve the conflicts as well
Added test coverage in existing kernel test instead of creating new test from performance perspective.
Please review, moving NR.
Addressed the mentioned feedback , only one point pending working on it when navigation module enable then tour should be place on the navigation by default
Addressed the feedback, rebased the MR, apart from it nothing seems to be left.
Please review, moving NR.
@benjifisher, I missed the point, like better solution is to add same tag in existing formatter instead of creating new one, thanks for the feedback, 'll try to keep address these points as well.
Try to addressed the requested changes , update issue summary as well. as we have used '-None- as by default so when I try save entity from "manage form" page then is no any error encountered , I guess no need to add upgrade path as well.
PLease review, moving NR.
Addressed the both requested changes, when user go to : /admin/config/user-interface/navigation-block?
when scroll to the left sidebar can able to see "Add content" link when click on popup open where "Navigation tour" block available in the list, when user click on it, the block on the sidebar, also if hover on "Navigation tour" contextual links available like: move, edit, attaching screenshots for the reference.
This one thing which I'm not sure need to address or not , currently to display "Tour button" on the navigation sidebar user need to place it manually, but the main goal of this issue is address.
if we want tour button should be placed by default when navigation module is installed, for this, better to create separate followup- requires additional effort.
Please review, moving NR.
Thanks for reviewing it, I agree , we can simply enhance the existing formatter rather than introducing new one. In proposed solution section written
Create a TitleFormatter
so I thought need create one.
I believe along with one mentioned feedback need upgrade path as well , as we are enhancing existing formatter. correct me I 'm missing anything that needs to be taken care.
I have concluded there for tour button in the context of navigation module, four things need to address:
- quick edit button (on hover) available with the options to configure
- remove block from navigation block from UI
- Add block from navigation block from UI
- Move block from navigation block from UI
Tried to add test coverage with super user policy disabled, tried to test the functionality of some piece of code of module , apart from it nothing seems to left , correct me if I 'm missing anything.
Please review, moving NR.
This is first time I have worked with nightwatch tests, tried to fix nightwatch test & left comment on MR -- mergeable.
Please review moving NR.
Thanks for reviewing, looking into it.
h6 tag by mistaken missed added back, Please review moving NR (usability review tag)
Tried to address the feedback. Please review, moving NR.
Thanks for reviewing it, looking into it.
Addressed this issue - If you collapse the navigation sidebar normally the labels are hidden and only the icons are shown. But for the tour block the label is still shown
however to fix first issue, I need to do bit R&D & revised whole logic, to make it work like other blocks for navigation.
I have added the permission for createUser() & removed the unrelated code for all test.
Please review, moving NR.
pooja_sharma → made their first commit to this issue’s fork.
Tried to address issue & added test coverage with minimal changes in relevant existing test.
Please review, moving NR.
I have taken the latest pull , however still replicable this issue, attaching the screenshots.
I tried to upload the navigation custom logo image
Then click on file menu on the left sidebar this page open, you 'll redirect to this page: admin/content/files
Click on the Usage link for this file , page break with above mentioned error.
Tried to debug while calling file usage API(), correct entity type missing that causing error, working on it.
Applied suggestions on MR & fixed test failures as well now MR is meregeable.
PLease review, moving NR.
Attached the screenshots as well for reference. PLease review, moving NR.
Integrated Admin toolbar "tour Button" with Navigation, tried to reuse the existing block rather than create new plugins so that code remain centralised , reused the existing navigation approach for rendering tour component so that existing navigation code remain sync, attached the screenshots as well for reference.
Please review, moving NR.
While working on this issue ✨ Integrate with the new experimental navigation module Fixed , analysing it's approach found this bug, fixed it & also fixed cache context API calling syntax LazyBuilders, which also caused the issue.
PLease review, moving NR.
pooja_sharma → created an issue.
Could a site builder end up creating a situation where they set the sub title to use h2 and then the hierarchy no longer matches between teaser and page views?
- Page
- h1: Node Title via page-title
- h2: Sub title (Default form mode)
- Teaser
- h1: Page title
- h2: Node titlender
- h2: Sub title or h3: Sub title (whatever selected in tag of title formatter in specific form mode like here teaser, 'll render on FE)
Yes I have verified it is possible, correct me if I 'm missing anything.
Yeah I have similar thoughts, u didn't mention similar changes here then also skip from my end as well.
Applied suggestions, PLease review, moving NR.
But logo_managed variable name for a path doesn't make much sense to me
Agree , 'll working on the same, keep logo_path variable name.
Logo_extensions seems to have no field (is there not a way to get allowed file uploads?)
Generally when we upload the image on the node form , there is only visible image field, & image extension not visible to the user , it is attached with field storage.
Here if we 'll add logo_extension field then on the same form logo extension & logo upload field both 'll be visible, this 'll be bit confusing experience for the user.
The schema for logo_managed says it's allowed to be null but if it's a path to an image then should validate it's a valid path.
On the basis of my understanding trying to rply correct me if I 'm missing anything, 'logo_managed' can be null because at the BE there are three opts available: default: (by default this value is selected when this selected then image path not get from the config, so image path not store as well, due to which I set null ) ,
hide: in this opt no image 'll display & not store it's value in config, custom logo update: only in this opt image store, so on the basis above mentioned scenarios keep it null.
fixed test failures of these two test: BookTest, BookUninstallTest, along with phpcs issue
But there is one test failure for following test:
testBookAdminLocalTasks on line no. 21 "'book' => 'modules/custom/book',
This test coverage break on local with similar error s, however update correct path 'book' => 'modules/contrib/book'
of this directory on local & it fixed
how we are thinking to fix this as now book is part of contrib module & just wondering why this issue coming not in other MR, why this is replicate here , any thoughts or suggestion
pooja_sharma → made their first commit to this issue’s fork.
@quietone, Thanks for recognizing my efforts & dedication, your support, it means a lot to me . I 'd also like to mention, community members are really helpful & supportive, it's been wonderful opportunity to contribute here.
It's amazing to get appreciation from you , when I start contribution few months ago this feedback I got ✨ Disabled update module shouldn't produce a status report warning Needs work at that time understand your concerns & I thought one day I definitely 'll get appreciation from you & here it is finally 📌 Fix Content Translation tests that rely on UID1's super user behavior Active ;-)
Thanks @smustgrave, @quietone for believing in me & guiding.
Addressed the feedback, Please review, moving NR.
While debugging I have comment root user code ContentTranslationRevisionTranslationDeletionTest, due to which at that time I think need to add perms for editor (as test html output of 272 pages due to this happen), tried to add perm for appropriate user role, the tried to compare the output (ContentTranslationRevisionTranslationDeletionTest) with & without diff it's same.
PLease review , moving NR.
Changes looks good to me, so moving to RTBC ++
@kumudb, This issue not replicate due to these MR changes , I have verified & created issue here 🐛 Page break error display :The "logo" entity type does not exist Active , can track there.
pooja_sharma → created an issue.
ContentTranslationTestBase which is used by several of these tests provides methods to assign the permissions for the administrator, editor and translator. I think we should continue to that approach and override those methods to assign the permissions in these tests. Such as is done in \Drupal\Tests\node\Functional\NodeTranslationUITest::getAdministratorPermissions().
Go through the whole code for suggested approach, used it, this addressed both reviewer feedback. Please review, move NR.
Tried to address the mentioned feedback, apart form it nothing appears to be left.
MR is mergeable, there is one warning validation config, it display validation count & line no. increase which is correct, no any error message displaying, not sure on the basis of it, PLease review, moving to NR.
pooja_sharma → created an issue.
hard coding values of logo extension now added in config & added constraints validation as well
As new key logo_extension
added, it leads to 'validation config' warning: https://git.drupalcode.org/issue/drupal-3462829/-/jobs/2613092
But not getting similar warning on local even try with this command: drush config:inspect --filter-keys=navigation.settings --detail --list-constraints
if 'll be great if anyone can share suggestion for it.
- "navigation.settings": 4,
+ "navigation.settings": 5,
"node.settings": 2,
"node.type.*": 22,
"olivero.settings": 16,
@@ -2788,10 +2788,10 @@
},
"perPropertyPath": {
"all": {
- "count": 17140
+ "count": 17141
},
"validatable": {
- "count": 10377
+ "count": 10379
},
"fullyValidatable": {
"count": -1
On adding validation constraints appears random test failures , moving to NW 'll debug it.
Addressed the feedback, left comment on MR. PLease review, moving NR.
Addressed the feedback, left comment on MR. PLease review, moving NR.
Not much work with NIghtwatch test can give it try, it 'll be great if you can share hints/suggestions.
pooja_sharma → made their first commit to this issue’s fork.
Thanks for reviewing , working on the it.
Thanks for reviewing. working on the feedback.
Addressed the feedback, left one comment on MR. PLease review, moving NR.
Ported the changes to 10.4.x, PLease review MR 9347
pooja_sharma → made their first commit to this issue’s fork.
I believe instead of updating title of this issue, we should fix this issue, can create separate meta ticket where can attach this issue link so that we can track where we exactly trace out these changes , may be further tackle these on a per module basis.
Looking into this however before start I 'd like to confirm regarding position of tour button to support tour for experimental navigation module. Should we display the "tour button" above "create" menu of navigation sidebar(attached the screenshot & follow #7 ✨ Integrate with Workspaces Active shared link)
Issue 2
The overrides do not work on at least: = Should be fixed.
/admin/config/user-interface/tour
/admin/config/user-interface/tour/settings
This issue is fixed as part of this Caching issue of config form settings 🐛 Caching issue of config form of Tour and No Tour available button labels Needs review ticket
PLease review, moving NR. attached screencast as well.
I have fixed the cache issue, now whenever user update tour labels, the changes reflect immediately on existing open pages & for all other pages as well.
PLease review, moving NR.
pooja_sharma → created an issue.
Added title formatter as mentioned in proposed solution & added test coverage as well for title formatter, apart form nothing seems to be left.
PLease review, moving NR
I have verified & applied changes working fine for me, on click on modal close button , "Take a tour button page" button highlight with grey color & attached the screencast as well moving to RTBC.