Skip to content

Conversation

imorland
Copy link
Member

@imorland imorland commented Nov 21, 2022

Implements Bounty proposal
Fixes: #3693

TODO:
[ ] #3620 Modify tag changed eventposts to mention the tags
[ ] Major cleanup
[ ] Add backend tests

Changes proposed in this pull request:

  • deprecated getMentionText in favour of a new MentionTextGenerator

Reviewers should focus on:

Screenshot
Early version (will likely change a few times yet)
image

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Required changes:

  • Related documentation PR: (Remove if irrelevant)
  • Related core extension PRs: (Remove if irrelevant)

@imorland imorland self-assigned this Nov 21, 2022
@imorland imorland added this to the 1.7 milestone Nov 21, 2022
Copy link
Contributor

@Ornanovitch Ornanovitch left a comment

Choose a reason for hiding this comment

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

Just a few remarks considering #3653 and the logic introduced with #3652

tag.setAttribute('icon', tagModel.icon());
tag.setAttribute('color', tagModel.color());
tag.setAttribute('slug', tagModel.slug());
tag.setAttribute('class', isDark(tagModel.color()) ? 'TagMention--light' : 'TagMention--dark');
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a little confusion here, it should be the contrary :)
Cf. #3653

Suggested change
tag.setAttribute('class', isDark(tagModel.color()) ? 'TagMention--light' : 'TagMention--dark');
tag.setAttribute('class', isDark(tagModel.color()) ? 'TagMention--dark' : 'TagMention--light');

Copy link
Member Author

Choose a reason for hiding this comment

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

In actual usage, I've found the opposite. See my dev forum for a live version of this..

Copy link
Contributor

@Ornanovitch Ornanovitch Nov 22, 2022

Choose a reason for hiding this comment

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

I know it's working perfectly well, this is not a dev problem I suppose. ✌️

What I meant is just that the semantical logic of isDark usage is (according to me) to ask is this a dark color ?, and if it's true, to apply the text-on-dark rule. Adding a --light suffix for a dark element class (or the opposite) sounds counterintuitive IMHO. This could lead to an issue of consistency about the usage of this isDark utility across the project

But maybe there is something I don't understand in your case, I'll try to test your branch!

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would be simpler to think about all this with #3653 merged. We could then try to find a homogeneous and common way of operating with those contrast issues :)

&:hover,
&:active {
color: @text-on-light;
color: @text-on-dark;
Copy link
Contributor

Choose a reason for hiding this comment

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

following my previous comment

Suggested change
color: @text-on-dark;
color: @text-on-light;

&:hover,
&:active {
color: @text-on-dark;
color: @text-on-light;
Copy link
Contributor

Choose a reason for hiding this comment

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

and so on

Suggested change
color: @text-on-light;
color: @text-on-dark;

@SychO9 SychO9 modified the milestones: 1.7, 1.8 Mar 6, 2023
@SychO9
Copy link
Member

SychO9 commented Mar 18, 2023

See #3769

@SychO9 SychO9 closed this Mar 18, 2023
@SychO9 SychO9 deleted the im/mention-tags branch March 18, 2023 22:47
@SychO9 SychO9 removed this from the 1.8 milestone Mar 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Mentions] 3 characters hex color and isDark util: Unsupported operand types: string + string

4 participants