Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ENH Add versioning to Link #90

Merged
merged 1 commit into from
Dec 11, 2023
Merged

ENH Add versioning to Link #90

merged 1 commit into from
Dec 11, 2023

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Sep 26, 2023 •
edited
Loading

Issue #7

@emteknetnz emteknetnz force-pushed the pulls/4/support-versioning branch from 72c1c40 to 5f526fb Compare September 26, 2023 20:42
@emteknetnz emteknetnz mentioned this pull request Sep 26, 2023
Closed
michalkleiner
michalkleiner reviewed Sep 26, 2023
View reviewed changes
_config.php Outdated Show resolved Hide resolved
michalkleiner
michalkleiner reviewed Sep 26, 2023
View reviewed changes
src/Models/Link.php Outdated Show resolved Hide resolved
@emteknetnz emteknetnz force-pushed the pulls/4/support-versioning branch 3 times, most recently from 66103e9 to b008bad Compare September 28, 2023 02:07
@emteknetnz emteknetnz changed the base branch from 3 to 4 September 28, 2023 06:06
@emteknetnz emteknetnz mentioned this pull request Oct 9, 2023
Closed
@maxime-rainville maxime-rainville mentioned this pull request Dec 4, 2023
Closed
1 task
@emteknetnz emteknetnz force-pushed the pulls/4/support-versioning branch 7 times, most recently from a2b0cda to 09f0108 Compare December 5, 2023 02:53
emteknetnz
emteknetnz commented Dec 5, 2023
View reviewed changes
tests/php/Models/LinkTest.php
@@ -250,14 +271,9 @@ public function linkTypeEnabledProvider(): array
*/
public function testGetUrl(string $identifier, string $class, string $expected): void
{
Versioned::withVersionedMode(function () use ($identifier, $class, $expected): void {
Versioned::set_stage(Versioned::LIVE);
Copy link
Member Author

@emteknetnz emteknetnz Dec 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Versioning doesn't have any impact on this test (it's testing getURL()), and setting things to LIVE means we'd need to publish the Links before making assertions

@emteknetnz
Copy link
Member Author

emteknetnz commented Dec 7, 2023 •
edited
Loading

Design feedback:

  • Keep icons as they are
  • Add versioned badges to the right of the descriptions, similar to what the site tree has
  • <span class="badge status-modified" title="Page has unpublished changes">Modified</span> #cf3f00 #fff2ea
  • Truncate title in PHP to a reasonable character limit (30?) and add '...' if needed
  • Raise a card to implement text badges in elemental

@emteknetnz emteknetnz force-pushed the pulls/4/support-versioning branch 2 times, most recently from 3f3483e to c2f8b7b Compare December 7, 2023 21:56
@emteknetnz
Copy link
Member Author

emteknetnz commented Dec 7, 2023 •
edited
Loading

Design review has implemented and approved

@emteknetnz emteknetnz marked this pull request as ready for review December 7, 2023 22:03
sabina-talipova
sabina-talipova suggested changes Dec 10, 2023
View reviewed changes
Copy link
Contributor

@sabina-talipova sabina-talipova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tiny changes.

README.md Outdated Show resolved Hide resolved
client/src/components/LinkPicker/LinkPickerTitle.js Outdated Show resolved Hide resolved
client/src/components/LinkPicker/LinkPickerTitle.js Outdated Show resolved Hide resolved
@emteknetnz emteknetnz force-pushed the pulls/4/support-versioning branch from c2f8b7b to 4ef6633 Compare December 11, 2023 01:54
@emteknetnz emteknetnz force-pushed the pulls/4/support-versioning branch from 4ef6633 to ffb1ae0 Compare December 11, 2023 01:57
sabina-talipova
sabina-talipova approved these changes Dec 11, 2023
View reviewed changes
Copy link
Contributor

@sabina-talipova sabina-talipova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Test locally. Works perfect.

@sabina-talipova sabina-talipova merged commit 9010e73 into silverstripe:4 Dec 11, 2023
10 checks passed
@sabina-talipova sabina-talipova deleted the pulls/4/support-versioning branch December 11, 2023 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michalkleiner michalkleiner michalkleiner left review comments

@sabina-talipova sabina-talipova sabina-talipova approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@emteknetnz @michalkleiner @sabina-talipova