-
Notifications
You must be signed in to change notification settings - Fork 18
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.
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
Related links #1054
base: dev/new_features
Are you sure you want to change the base?
Related links #1054
Conversation
Limit it to links would be fine for me, but I don't think that it makes sense 🤔 |
|
||
#addButtonDeleteLink(item) { | ||
const removeFormButton = document.createElement('button'); | ||
removeFormButton.innerText = '⌫'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think overwriting innerHtml
with the font awesome trashcan icon (https://fontawesome.com/icons/trash?f=classic&s=solid) would be better. Then you could also make it a button:
<div class="btn btn__secondary">
<i class="fa-solid fa-trash"></i>
</div>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified
@@ -894,4 +897,14 @@ public function canUpdateUser(User $actor): bool | |||
return $this->apDomain === $actor->apDomain; | |||
} | |||
} | |||
|
|||
public function getRelatedLinks(): array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add phpDoc so we know which kind of array is returned? I really liked if the returned value would be of RelatedLink[]
so an array of objects, so we can add some validation logic so we can mark links as "checked" or something so we know that the rel=me is actually correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed Discussed on element chat.
I don't know a elegant way to denormalize the json field from table and return it on get method. This'll required some dependencies inside entity that I think is not a good practice.
Instead, I created a RelatedLinkDTO class and this will be returned in UserDTO.
src/Form/UserRelatedDataType.php
Outdated
{ | ||
$builder | ||
->add('label', TextType::class) | ||
->add('link', UrlType::class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The url type force this value to be a url. Even if I insert something normal, it still comes out as a url. In general I think this variable/field should be called value
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modified link by value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love if the add button would something like this (but that is not a requirement):
I noticed that the links are not yet displayed on the profile page. Are you planning on adding that in this PR?
(The activity pub representation of the fields is also missing, but I expected to do that myself anyways :) but if you'd like to try that as well, feel free to do so)
src/DTO/RelatedLinkDTO.php
Outdated
class RelatedLinkDTO | ||
{ | ||
private string $label; | ||
private string $link; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
private string $link; | |
private string $value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modified
src/DTO/RelatedLinkDTO.php
Outdated
{ | ||
private string $label; | ||
private string $link; | ||
private bool $verified = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private bool $verified = false; | |
private bool $verifiedLink = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modified
src/Entity/User.php
Outdated
return $this->relatedLinks; | ||
} | ||
|
||
public function setRelatedLinks(array $relatedLinks): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is actually not used. Maybe make the variable private and use this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deleted
src/Factory/UserFactory.php
Outdated
// $temp = $this->denormalizer->denormalize($user->getRelatedLinks(), sprintf('%s[]', RelatedLinkDTO::class)); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deleted
this.indexValue++; | ||
} | ||
|
||
#addButtonDeleteLink(item) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this hashtag for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Define a method private
https://railsdesigner.com/proper-stimulus-controllers/
Could be added on a utility file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assets/styles/layout/_forms.scss
Outdated
.related-link-row button { | ||
padding: 0; | ||
background: none; | ||
font-size: 1.5rem; | ||
border: 0; | ||
color: var(--kbin-input-text-color); | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove that and add the class btn__secondary
to the trash icon buttons. Then they will look like other buttons in our UI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modified
translations/messages.en.yaml
Outdated
@@ -890,3 +890,4 @@ admin_users_suspended: Suspended | |||
admin_users_banned: Banned | |||
user_verify: Activate account | |||
max_image_size: Maximum file size | |||
related_links: Related links |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep newlines at the end of files
Oh and please change the target of the PR to |
edc97fa
to
481b0f7
Compare
ffbdc54
to
98ed7e1
Compare
Added new dynamic field in profile section witch let us to add related links.
98ed7e1
to
3f043fd
Compare
cb071ab
to
68d6d11
Compare
First of all thanks for your time and effort ❤️ From your comment and screenshots I (already) have a few remarks (sorry 🙈):
I think I did it by making a div with a margin left the same size as the buttons and then just put
What happens when the value is not a link? |
There is a conflict with |
Added new dynamic field in profile section witch let us to add related links.
#839