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

web4: scroll comments into view #142

Merged

Conversation

petersalomonsen
Copy link
Collaborator

@petersalomonsen petersalomonsen commented Jul 10, 2024

Enables "scroll into view" for proposal comments functionality for NEAR-DevHub/neardevhub-bos#868

Example link to comment being scrolled into view: https://devhublink.testnet.page/proposal/127?accountId=theori.near&blockHeight=121684702

@petersalomonsen
Copy link
Collaborator Author

@race-of-sloths please, include my PR in the race

@race-of-sloths
Copy link

race-of-sloths commented Jul 12, 2024

@petersalomonsen Thank you for your contribution! Your pull request is now a part of the Race of Sloths!
New Sloth joined the Race! Welcome!

Shows profile picture for the author of the PR

Current status: executed
Reviewer Score
@frol 3

The average score is 3

@petersalomonsen check out your results on the Race of Sloths Leaderboard! and in the profile

What is the Race of Sloths

Race of Sloths is a friendly competition where you can participate in challenges and compete with other open-source contributors within your normal workflow

For contributors:

  • Tag @race-of-sloths inside your pull requests
  • Wait for the maintainer to review and score your pull request
  • Check out your position in the Leaderboard
  • Keep weekly and monthly streaks to reach higher positions
  • Boast your contributions with a dynamic picture of your Profile

For maintainers:

  • Score pull requests that participate in the Race of Sloths
  • Engage contributors with fair scoring and fast responses so they keep their streaks
  • Promote the Race to the point where the Race starts promoting you
  • Grow the community of your contributors

Feel free to check our website for additional details!

Bot commands
  • For contributors
    • Include a PR: @race-of-sloths include to enter the Race with your PR
  • For maintainers:
    • Assign points: @race-of-sloths score [1/2/3/5/8/13] to award points based on your assessment.
    • Reject this PR: @race-of-sloths exclude to send this PR back to the drawing board.
    • Exclude repo: @race-of-sloths pause to stop bot activity in this repo until @race-of-sloths unpause command is called

Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

@petersalomonsen Can you provide an example link to test?

src/web4/scroll_comment_into_view.js Show resolved Hide resolved
@petersalomonsen
Copy link
Collaborator Author

@petersalomonsen Can you provide an example link to test?

yeah, you can also find it in the frontend PR as referred to in the description, but here is a link the deployment on testnet ( with a frontend that points to mainnet ): https://devhublink.testnet.page/proposal/127?accountId=theori.near&blockHeight=121684702

@Tguntenaar
Copy link
Collaborator

The preview does not work for me:
Uploading Screenshot 2024-07-18 at 17.35.46.png…

Copy link
Collaborator

@Tguntenaar Tguntenaar left a comment

Choose a reason for hiding this comment

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

@frol @petersalomonsen quick question:

For the tracking of referral sources we need to have access to the PostHog API key inside the javascript. I think the best way is to expose the env variable to the rust build process and replace part of the imported javascript string.

Do you guys agree?

@frol
Copy link
Collaborator

frol commented Jul 18, 2024

@Tguntenaar I find it not the best solution:

  1. It makes the contract build "not reproducible" without the knowledge to the API key (cargo-near reproducible builds do not even support env variables passing, so I would avoid that)
  2. There is too much of indirection for this API key to travel from github build process of a contract down to web4 gateway JS code
  3. We already have the key in our widgets
  4. @petersalomonsen Can we pass extra variable to the BOS components via context (next to context.networkId) or maybe at least via props (e.g. implicitly passing ?gateway=devhub.near.page when rendering the top-level component)?

@petersalomonsen
Copy link
Collaborator Author

@Tguntenaar the preview link works now ( it was pointing to devgovgigs.petersalomonsen.near preview environment which I have deleted, now it is pointing to devhub.near )

@frol @Tguntenaar Regarding tracking the gateway being used. We don't need to include it in the contract build process.

There are other alternatives, and the easiest is to pass the additional information we want to track into props. Then it will be automatically visible in PostHOG too.

Passing into context seems not to be supported out of the box in the VM, so I think props is better then.

We can also go into more "hacky" alternatives like extracting the key from the widget, but I would prefer just passing the information we want to track into props.

And finally, regarding this PR. The solution on looking for an element id to scroll to, is an attempt to create a generic solution for finding such elements that can be scrolled to. An alternative would be to lock it to only proposals, and only then wait for the element to appear. In that case we need to revisit this every time we want to support scrolling to a comment element for something else than proposals, so I would prefer the current solution @frol .

@Tguntenaar
Copy link
Collaborator

Tguntenaar commented Jul 19, 2024

Thanks for the quick feedback guys, @petersalomonsen the preview looks good to me!

Just for reference the issue we are talking about #879

@Tguntenaar Tguntenaar self-requested a review July 19, 2024 09:55
Copy link
Collaborator

@Tguntenaar Tguntenaar left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

@race-of-sloths score 3

@frol frol merged commit 7673833 into NEAR-DevHub:main Jul 19, 2024
1 check passed
@race-of-sloths
Copy link

🌟 Score recorded!

@frol, thank you for scoring this pull request in the Race of Sloths!

@race-of-sloths
Copy link

✅ PR is finalized!

Your contribution is much appreciated with a final score of 3!
You have received 45 (30 base + 15 weekly bonus) Sloth points for this contribution

Another weekly streak completed, well done @petersalomonsen! To keep your weekly streak and get another bonus make pull request next week! Looking forward to see you in race-of-sloths

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