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.

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

Alter attribute input for mentions #720

Merged
merged 23 commits into from
Jun 16, 2023

Conversation

alunturner
Copy link
Contributor

@alunturner alunturner commented Jun 14, 2023

This PR:

  • "unhooks" the attributes from the mobile side of things - they no longer need to pass in any attributes as per this issue Remove attribute argument from calls to mention functions  #721, but still require removal from the mobile side of the codebase
  • makes the addition of data-mention-type automatic
  • updates the tests to reflect this new behaviour
  • stops web passing in the unneeded data-mention-type attribute in web, and changes the type for attributes to a map
  • associated event/type changes in web to allow us to get the formatted message output and also to handle at room mentions using the new exposed api function

@codecov-commenter
Copy link

codecov-commenter commented Jun 14, 2023

Codecov Report

Patch coverage: 75.00% and project coverage change: -0.18 ⚠️

Comparison is base (4f375fc) 90.04% compared to head (2ed5d77) 89.87%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #720      +/-   ##
==========================================
- Coverage   90.04%   89.87%   -0.18%     
==========================================
  Files         113       83      -30     
  Lines       16059    14603    -1456     
  Branches      565        0     -565     
==========================================
- Hits        14460    13124    -1336     
+ Misses       1579     1479     -100     
+ Partials       20        0      -20     
Flag Coverage Δ
uitests ?
uitests-ios ?
unittests 89.87% <75.00%> (+1.09%) ⬆️
unittests-ios ?
unittests-rust 89.87% <75.00%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
crates/wysiwyg/src/dom/parser/parse.rs 97.90% <ø> (ø)
bindings/wysiwyg-ffi/src/ffi_composer_model.rs 28.02% <20.00%> (+0.56%) ⬆️
bindings/wysiwyg-ffi/src/ffi_composer_update.rs 98.26% <100.00%> (-0.03%) ⬇️
crates/wysiwyg/src/dom/nodes/mention_node.rs 99.38% <100.00%> (+0.04%) ⬆️

... and 30 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

aringenbach

This comment was marked as off-topic.

@alunturner alunturner force-pushed the alunturner/alter-attribute-input-for-mentions branch 2 times, most recently from 140d97a to e2b8344 Compare June 15, 2023 17:07
@alunturner alunturner force-pushed the alunturner/alter-attribute-input-for-mentions branch from 729f5ba to b1ca983 Compare June 16, 2023 10:45
@alunturner alunturner marked this pull request as ready for review June 16, 2023 10:45
Comment on lines -1084 to -1100
#[wasm_bindgen_test]
fn a_with_attributes() {
roundtrip(
r#"<a contenteditable="false" data-mention-type="user" style="something" href="http://example.com">a user mention</a>"#,
);
}

#[wasm_bindgen_test]
fn a_with_bad_attribute() {
let html = r#"<a invalidattribute="true" href="http://example.com">a user mention</a>"#;
let dom = HtmlParser::default().parse::<Utf16String>(html).unwrap();
assert_eq!(
dom.to_string(),
r#"<a href="http://example.com">a user mention</a>"#
);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer good tests, as we don't have links with attributes

Comment on lines +75 to +92
case 'insertAtRoomSuggestion': {
if (suggestion && isAtRoomSuggestionEvent(event)) {
const { attributes } = event.data;
// we need to track data-mention-type in element web, ensure we do not pass
// it in as rust model can handle this automatically
if (attributes.has('data-mention-type')) {
attributes.delete('data-mention-type');
}
return action(
composerModel.insert_at_room_mention_at_suggestion(
suggestion,
attributes,
),
'insert_at_room_mention_at_suggestion',
);
}
break;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This replaces the if text === ... check from before to completely separate mention and at mention handling

Comment on lines +29 to +32
export type AllowedMentionAttributes = Map<
'style' | 'data-mention-type',
string
>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change the type to be a Map, which fixes another open issue and we also make it less permissive in terms of what it accepts.

@@ -108,5 +108,6 @@ export function useWysiwyg(wysiwygProps?: WysiwygProps) {
traceAction: testUtilities.traceAction,
},
suggestion: memoisedMappedSuggestion,
messageContent: composerModel?.get_content_as_message_html() ?? null,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exposing the ability to use the rust model to output message format html

Copy link
Contributor

@jonnyandrew jonnyandrew left a comment

Choose a reason for hiding this comment

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

I'm happy to see this change happen!

@@ -831,8 +831,8 @@ mod js {
self.current_path.push(DomNodeKind::Link);

let mut attributes = vec![];
let valid_attributes =
["contenteditable", "data-mention-type", "style"];
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

@alunturner alunturner force-pushed the alunturner/alter-attribute-input-for-mentions branch from 2ed5d77 to 42300cd Compare June 16, 2023 12:26
@sonarcloud
Copy link

sonarcloud bot commented Jun 16, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@alunturner alunturner merged commit 1bf905d into main Jun 16, 2023
6 of 8 checks passed
@alunturner alunturner deleted the alunturner/alter-attribute-input-for-mentions branch June 16, 2023 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants