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

fix: mf-5058 missing tips in mirror post page #10768

Merged
merged 3 commits into from
Sep 18, 2023
Merged

fix: mf-5058 missing tips in mirror post page #10768

merged 3 commits into from
Sep 18, 2023

Conversation

UncleBill
Copy link
Contributor

@UncleBill UncleBill added this to the 2.23.0 milestone Sep 15, 2023
@UncleBill UncleBill marked this pull request as draft September 15, 2023 02:55
@github-actions github-actions bot temporarily deployed to pull request September 15, 2023 02:58 Inactive
@UncleBill UncleBill marked this pull request as ready for review September 18, 2023 03:17
@github-actions github-actions bot temporarily deployed to pull request September 18, 2023 03:22 Inactive
@github-actions github-actions bot temporarily deployed to pull request September 18, 2023 03:26 Inactive
@UncleBill UncleBill marked this pull request as draft September 18, 2023 03:41
@github-actions github-actions bot temporarily deployed to pull request September 18, 2023 03:41 Inactive
@UncleBill UncleBill marked this pull request as ready for review September 18, 2023 03:43
guanbinrui
guanbinrui reviewed Sep 18, 2023
View reviewed changes
packages/mask/src/site-adaptors/mirror.xyz/injection/Tips/MenuAuthorTipButton.tsx Outdated
function selector() {
return querySelector(
[
'div[style$="height: 56px;"] a',
Copy link
Member

Choose a reason for hiding this comment

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

Matching a style attribute is easy to 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.

Then will fallback to div:has(> div > button[data-state="closed"]) a which is more relaible.

Copy link
Contributor Author

@UncleBill UncleBill Sep 18, 2023 •
edited

Choose a reason for hiding this comment

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

Just moved div:has(> div > button[data-state="closed"]) a to the first.

packages/mask/src/site-adaptors/mirror.xyz/injection/Tips/PostVerification.tsx Show resolved Hide resolved
packages/mask/src/site-adaptors/mirror.xyz/injection/Tips/ProfilePage.tsx
// Only the address link
return querySelector(
[
`#__next div:has(h2) ~ div a[href$="/address/${authorWallet}" i]`, // address
Copy link
Member

Choose a reason for hiding this comment

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

Is authorWallet case sensitive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto

packages/mask/src/site-adaptors/mirror.xyz/injection/Tips/TipsButtonWrapper.tsx
const isDashboard = location.href === '/'
if (!component || !visitingIdentity.identifier || isMinimal || isDashboard) return null

return (
Copy link
Member

Choose a reason for hiding this comment

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

It could yield HTML contains span wraps block elements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's only the tip button, and elements inside are isolated.

BTW span > div is valid.

packages/mask/src/site-adaptors/mirror.xyz/utils/selectors.ts
export const postsContentSelector = () =>
querySelectorAll(
[
// In Entries
'[id="__next"] > div:nth-child(2) > div > div:not([class]) > div:not(footer)',
// In collection
'[id="__next"] > div:nth-child(2) > div > div > div > a:has(footer)',
'[id="__next"] > div:nth-child(2) a:has(footer)',
'[id="__next"] > div:nth-child(2) a:has(img[alt="Card Header"][loading="lazy"])',
Copy link
Member

Choose a reason for hiding this comment

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

Attention i18n alternative text will fail the selector.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Mirror doesn't have yet.
  2. It's hardcoded
image

Copy link
Member

Choose a reason for hiding this comment

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

LOL if this is not the only selector, maybe it's ok

@github-actions github-actions bot temporarily deployed to pull request September 18, 2023 03:45 Inactive
@github-actions github-actions bot temporarily deployed to pull request September 18, 2023 03:52 Inactive
@guanbinrui guanbinrui merged commit 4d491a6 into develop Sep 18, 2023
15 checks passed
@guanbinrui guanbinrui deleted the fix/mf-5058-mirror-tip branch September 18, 2023 04:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jack-Works Jack-Works Jack-Works left review comments

@guanbinrui guanbinrui guanbinrui approved these changes

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@UncleBill @Jack-Works @guanbinrui