Simplify and compact curation log table#2400
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes update table styling for compactness and dark text, enhance Bootstrap tooltip initialization, add a tooltip method to the curation log model, restructure the curation log grid view to use tooltips and condensed columns, and update acceptance tests and step definitions to match the new UI and tooltip content. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant View (curationLog.php)
participant Model (CurationLog)
participant Tooltip (Bootstrap)
User->>View (curationLog.php): Loads curation log page
View (curationLog.php)->>Model (CurationLog): Calls getTooltip() for each row
Model (CurationLog)-->>View (curationLog.php): Returns encoded tooltip HTML
View (curationLog.php)-->>Tooltip (Bootstrap): Initializes tooltip with HTML content
User-->>Tooltip (Bootstrap): Hovers over table row
Tooltip (Bootstrap)-->>User: Displays tooltip with creator/modification info
Possibly related issues
Suggested labels
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
| color: @color-darker-gray; | ||
| } | ||
| // use if table background color is not white | ||
| table.table--black-text td { |
There was a problem hiding this comment.
this style helps darken the text of tables with gray background
| } | ||
| } | ||
|
|
||
| table.table-compact { |
There was a problem hiding this comment.
styles for a compact table that removes as much spacing a possible
| $(document).ready(function () { | ||
| $('[data-toggle="tooltip"]').tooltip(); | ||
| $('[data-toggle="tooltip"]').tooltip({ | ||
| html: true, |
There was a problem hiding this comment.
html: true parses tooltip content as html, allowing to show <br/>
container: body appends tooltip as direct child of the body. This is necessary to prevent breaking table layout as tooltip would be appended in between rows, creating table styling issues
Adding these options to all tooltips seems like an improvement. If tooltip content is plain text, nothign changes. Appending tooltip to body does not change its position (bootstrap handles positioning)
| 'value' => function($data) { | ||
| if (preg_match('/^<\?xml/', $data->comments)) { | ||
| return LogCurationFormatter::getDisplayXmlAttr($data->id, $data->comments); | ||
| $commentOutput = $data->comments; |
There was a problem hiding this comment.
this extended logic takes care of prepending action label if it exists, and also takes into account whether a comment exists, to properly format the output as follows:
i.e. case: output
- comment with no action:
[comment] - comment + action:
[action]: [comment] - action, no comment:
[action]
| Then I am on "/adminDataset/update/id/8" | ||
| And I wait "3" seconds | ||
| And I should see "Joe Bloggs" | ||
| And I should see "Created by: Joe Bloggs" in the source |
There was a problem hiding this comment.
look into the source directly, because the tooltip is not displayed by default
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
protected/models/CurationLog.php (1)
198-208: Remove unused variables for cleaner code.The method works correctly but contains unused variables flagged by static analysis.
Apply this diff to remove the unused variables:
public function getTooltip(): string { - $fmt = Yii::app()->format; - - $action = CHtml::encode($this->action); $createdBy = CHtml::encode($this->created_by); $modifiedBy = CHtml::encode($this->last_modified_by ?? '-'); $modifiedDate = CHtml::encode($this->last_modified_date ?? '-'); return "Created by: {$createdBy}<br/>Modified by: {$modifiedBy}<br/>Modified date: {$modifiedDate}"; }protected/views/adminDataset/curationLog.php (1)
26-43: Approve with minor suggestion for robustnessThe logic correctly implements the requirements to prepend action text to comments. The XML detection and formatting using
LogCurationFormatter::getDisplayXmlAttris appropriate.Consider adding null safety for the action text:
- $actionText = $data->action; + $actionText = $data->action ?? '';This prevents potential issues if the action field is null, though it may not be necessary depending on your data model constraints.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
less/modules/tables.less(2 hunks)protected/js/bootstrap-tooltip-init.js(1 hunks)protected/models/CurationLog.php(1 hunks)protected/views/adminDataset/curationLog.php(1 hunks)tests/_support/AcceptanceTester.php(1 hunks)tests/acceptance/AdminDatasetCurationLog.feature(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`**/*`: - Focus on code smells, logic errors, edge cases, missing test cases, se...
**/*: - Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues
- Avoid commenting on minor issues such as linting, formatting and style issues
- Never commit secrets, PII, passwords or API keys—use env-vars and .env files.
- Flag any file >300 lines or function >30 lines and suggest refactoring.
- Every new feature or bug-fix must arrive with automated tests; warn if none are touched.
- If the PR includes changes to the UI, the style guide should be updated too; warn if not.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
protected/js/bootstrap-tooltip-init.jstests/_support/AcceptanceTester.phptests/acceptance/AdminDatasetCurationLog.featureless/modules/tables.lessprotected/models/CurationLog.phpprotected/views/adminDataset/curationLog.php
`**/*.js`: - New JavaScript must live in `giga/app/client/js/` (not in `protecte...
**/*.js: - New JavaScript must live ingiga/app/client/js/(not inprotected/js/).
- Write ES-module code (
import…export) only—no global scripts.- Functions needing >2 logical params must take a single options object.
- Rely on jQuery 3 or Vue for DOM work; avoid direct DOM APIs unless impossible.
- No Console.log
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
protected/js/bootstrap-tooltip-init.js
`**/*.php`: - HTML follows web accessibility WCAG 2 standards
**/*.php: - HTML follows web accessibility WCAG 2 standards
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
tests/_support/AcceptanceTester.phpprotected/models/CurationLog.phpprotected/views/adminDataset/curationLog.php
`**/*.less`: - Follow SMACSS: - Base: Default element styles - Layout: `.l-*...
**/*.less: - Follow SMACSS:
- Base: Default element styles
- Layout:
.l-*classes for page skeleton- Modules:
.my-module-*for reusable components- State:
.is-*for UI states- Theme:
.theme-*for theme overrides- Never hard-code colors, always use color variables defined in
less/base/variables.less.- Desktop-first: media queries for mobile styles.
- Limit nesting to three levels; favour flat selectors for maintainability.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
less/modules/tables.less
🧠 Learnings (2)
📓 Common learnings
Learnt from: rija
PR: gigascience/gigadb-website#2157
File: CHANGELOG.md:7-7
Timestamp: 2025-05-26T17:52:49.544Z
Learning: In the GigaDB project, textual changes to website content should be categorized as "Feat" (feature) entries in the CHANGELOG.md, not as "Docs" (documentation) entries, even when the changes involve updating guidance or instructional text on web pages.
Learnt from: alli83
PR: gigascience/gigadb-website#2301
File: protected/views/user/_form.php:179-187
Timestamp: 2025-05-23T02:02:48.284Z
Learning: The PR "Fix 561 separate admin user page from non admin user page" focuses specifically on separating admin user pages from non-admin user pages, removing terms and captcha requirements on update and change password pages, and adding token generation for activation links. Issues outside this scope should be addressed in separate PRs.
tests/acceptance/AdminDatasetCurationLog.feature (1)
Learnt from: rija
PR: gigascience/gigadb-website#2157
File: CHANGELOG.md:7-7
Timestamp: 2025-05-26T17:52:49.544Z
Learning: In the GigaDB project, textual changes to website content should be categorized as "Feat" (feature) entries in the CHANGELOG.md, not as "Docs" (documentation) entries, even when the changes involve updating guidance or instructional text on web pages.
🧬 Code Graph Analysis (1)
protected/views/adminDataset/curationLog.php (1)
protected/components/LogCurationFormatter.php (2)
LogCurationFormatter(5-17)getDisplayXmlAttr(8-16)
🪛 PHPMD (2.15.0)
protected/models/CurationLog.php
200-200: Avoid unused local variables such as '$fmt'. (Unused Code Rules)
(UnusedLocalVariable)
202-202: Avoid unused local variables such as '$action'. (Unused Code Rules)
(UnusedLocalVariable)
🔇 Additional comments (8)
protected/js/bootstrap-tooltip-init.js (1)
2-5: LGTM! Tooltip configuration enhances functionality.The added configuration options properly support HTML content in tooltips and ensure proper positioning by appending to the body.
less/modules/tables.less (2)
14-16: LGTM! Text color utility for gray backgrounds.The
.table--black-textclass appropriately addresses text visibility on non-white table backgrounds.
292-304: LGTM! Compact table styling is well-implemented.The
.table-compactclass effectively reduces spacing while maintaining readability with appropriate padding and line-height values.tests/acceptance/AdminDatasetCurationLog.feature (1)
34-42: LGTM! Test properly validates tooltip content.The test correctly checks for tooltip content in the page source and includes proper issue tracking with the
@issue-2331tag.tests/_support/AcceptanceTester.php (1)
455-461: LGTM! Simple and effective step definition.The new step method properly wraps the existing
seeInSourcefunctionality for use in Gherkin scenarios.protected/views/adminDataset/curationLog.php (3)
14-14: LGTM - CSS classes added for compact stylingThe addition of
table-compactandtable--black-textclasses aligns with the PR objective to make the layout more compact and improve text readability.
15-15: LGTM - Bootstrap tooltip configurationThe tooltip configuration correctly implements the hover functionality to display detailed information from the removed columns. The syntax for
rowHtmlOptionsExpressionis appropriate for Yii framework grid views.
18-21: LGTM - Column structure enhancementConverting the creation_date column from string to array format with width specification is a valid improvement for better layout control.
luistoptal
changed the title
1 participant
Pull request for issue: #2331
Updated curator log table in dataset forms
How to test?
docker-compose run --rm codecept run --no-redirect -g issue-2331 acceptance -vvshould passHow have functionalities been implemented?
Any issues with implementation?
--
Any changes to automated tests?
Updated a breaking acceptance test
Summary by CodeRabbit
New Features
Enhancements
Tests