Fix CMS UI #25900
Fix CMS UI #25900
Conversation
|
⚪ ⚪ Ya make output | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
🟢 |
|
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the CMS UI navigation code to use centralized functions for managing tab navigation and URL hash updates. The changes eliminate direct jQuery tab manipulation and URL hash setting in favor of reusable utility functions.
- Extract tab navigation logic into reusable
setTab,showTab, andsetLocationfunctions - Update existing code to use the new centralized functions instead of direct jQuery calls
- Improve the default tab selection logic to use the currently active tab instead of hardcoding 'configs'
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| ydb/core/cms/ui/cms.js | Adds centralized tab navigation functions and refactors main() to use them |
| ydb/core/cms/ui/configs.js | Updates direct jQuery tab call to use the new setTab function |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| $('.nav-tabs a').on('shown.bs.tab', function (e) { | ||
| Parameters.page = e.target.hash.substr(1); | ||
| window.location.hash = $.param(Parameters); | ||
| setLocation({page: e.target.hash.substr(1)}); |
There was a problem hiding this comment.
The setLocation call only sets the page parameter, but the original code preserved all existing Parameters. This change will lose other URL parameters like show when switching tabs.
| setLocation({page: e.target.hash.substr(1)}); | |
| // Preserve all existing parameters, only update 'page' | |
| var currentParams = {}; | |
| if (window.location.hash.length > 1) { | |
| window.location.hash.substr(1).split('&').forEach(function(o) { | |
| var a = o.split('='); | |
| if (a[0]) currentParams[a[0]] = decodeURIComponent(a[1]); | |
| }); | |
| } | |
| currentParams.page = e.target.hash.substr(1); | |
| setLocation(currentParams); |
Copilot uses AI. Check for mistakes.
| for (var i = 0; i < activeTabs.length; ++i) { | ||
| setTab({page: activeTabs[i].hash.substr(1)}); | ||
| break; |
There was a problem hiding this comment.
[nitpick] The loop with immediate break is unnecessarily complex. Consider using activeTabs.first() or activeTabs[0] directly since you only need the first active tab.
| for (var i = 0; i < activeTabs.length; ++i) { | |
| setTab({page: activeTabs[i].hash.substr(1)}); | |
| break; | |
| if (activeTabs.length > 0) { | |
| setTab({page: activeTabs[0].hash.substr(1)}); |
Copilot uses AI. Check for mistakes.
Changelog entry
...
Changelog category
Description for reviewers
...