-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
Change public accounts pages to mount the web UI #19319
Conversation
4c79d60
to
373ef76
Compare
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.
git rm spec/controllers/remote_follow_controller_spec.rb
373ef76
to
f7a1399
Compare
Once again, please do break such PRs in multiple PRs or at least multiple commits. Not by any means a full review, but I can see at least two outstanding issues with this PR:
|
- When logged in, serve web app - When logged out, redirect to permalink - Fix `app-body` class not being set sometimes due to name conflict
f8ee184
to
d37df7f
Compare
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 can confirm the crash has been fixed. The routing situation is better, but still leads to the awkward situation that you can navigate to /@foo@bar
(a remote account displayed in the WebUI) or /@foo@bar/1234
(a remote status displayed in the WebUI) from the logged-out WebUI but refreshing the page (or re-loading an unloaded page) will unexpectedly redirect you to the remote server.
I'm not sure what the best solution is here, as all the alternatives I can imagine have their own drawback:
- not redirecting allows sharing links to the cached version of a post
- opening the remote content in a new tab rather than allowing navigation to the cached version of it may not be as smooth as the current logged-out UI is. But maybe it's not so bad, that's what we had until you changed the logged-out UI to use the WebUI
Apart from that, I'm confused about some of the noindex
uses. I have added inline comments.
I still have more general reservations about getting rid of server-side-rendered pages, in that:
- pages that did not require Javascript now require a good amount of it being loaded before showing anything. This means longer loading times, and it also means people disabling Javascript or with an incompatible browser (while we strive to maintain compatibility with a large amount of browsers, incompatibilities do occur) just won't see posts that people may share with them
- while I do think this reduces confusion about there being two views for you on your own server, I'm afraid it would bring more confusion when visiting remote servers. But maybe I'm wrong about that, and I'm not sure there's any way to know for sure except trying it at scale…
before_action :set_instance_presenter | ||
|
||
def show | ||
expires_in 0, public: true unless user_signed_in? | ||
end | ||
|
||
private | ||
|
||
def set_instance_presenter | ||
@instance_presenter = InstancePresenter.new | ||
end |
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.
How is this related?
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.
There was a crash because this presenter is used by the OpenGraph partial
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.
ok, but I'm not sure how adding the OpenGraph partial to this controller is related to begin with
|
||
<Helmet> | ||
<meta name='robots' content='noindex' /> | ||
</Helmet> |
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.
Isn't the MissingIndicator
always used while status and accounts haven't loaded yet? In this case, this would cause pages to never get indexed.
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.
Good point, loading indicator for the status page would be a good thing...
@@ -134,6 +134,7 @@ class Account < ApplicationRecord | |||
:role, | |||
:locale, | |||
:shows_application?, | |||
:prefers_noindex?, |
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 name sounds slightly weird to me as it's not that you prefer noindex, it's that you want noindex, but ok.
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 used this wording because there's no actual way to enforce whether a search engine indexes something or not.
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.
Right, but you can ensure you output noindex
.
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.
My concerns about requiring Javascript, as well as refreshing the WebUI unexpectedly redirecting to remote URLs remain. The issues with noindex
for statuses seem solved, but that's not the case for accounts.
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 believe the issues with noindex
have been resolved, my only remaining concerns are refreshing the WebUI unexpectedly redirecting to remote URLs, and more general concerns about requiring the use of Javascript.
Howdy. Was trying to understand what the 'Feature on profile' option does in the UI, and stumbled across the endorsements API. After some backtracking through the tree, it seems that this change accidentally deleted the visualization from the UI: Seems this feature is no more as of this change, or it's not in a place I can find it :) : #8146 |
Yeah, it's currently not anywhere, unfortunately :/ |
* Change public accounts pages to mount the web UI * Fix handling of remote usernames in routes - When logged in, serve web app - When logged out, redirect to permalink - Fix `app-body` class not being set sometimes due to name conflict * Fix missing `multiColumn` prop * Fix failing test * Use `discoverable` attribute to control indexing directives * Fix `<ColumnLoading />` not using `multiColumn` * Add `noindex` to accounts in REST API * Change noindex directive to not be rendered by default before a route is mounted * Add loading indicator for detailed status in web UI * Fix missing indicator appearing while account is loading in web UI
* Change public accounts pages to mount the web UI * Fix handling of remote usernames in routes - When logged in, serve web app - When logged out, redirect to permalink - Fix `app-body` class not being set sometimes due to name conflict * Fix missing `multiColumn` prop * Fix failing test * Use `discoverable` attribute to control indexing directives * Fix `<ColumnLoading />` not using `multiColumn` * Add `noindex` to accounts in REST API * Change noindex directive to not be rendered by default before a route is mounted * Add loading indicator for detailed status in web UI * Fix missing indicator appearing while account is loading in web UI # Conflicts: # app/serializers/rest/account_serializer.rb # app/views/statuses/_detailed_status.html.haml # app/views/statuses/_simple_status.html.haml
I don't understand this at all. The new interaction modal requires one to open a new tab/window to their own instance, copy the given URL, paste it into the search box before they can interact with any post. This is much, much less practical than the old modals that just asked me for my handle and then remembered it, so that any interaction takes two clicks. Please at least provide a way to revert to the old behaviour, which should be the default IMO. |
/web
prefix from web app (maintain redirect)HomeController
, we make an exception whenrequest.path == '/'
)This finally completes #16800
Note about
robots
meta tag handling:react-helmet
does not overwrite header tags that were already in the HTML, it only appends new ones. That means if the HTML sets onerobots
tag, it will be present on any subsequent SPA navigation, even if other routes append their ownrobots
tag. I am not 100% sure how such a case is handled by search engines -- which tag is prioritized, the first, or the last. That being said, according to Google's documentation, Google will only render and process the page's JavaScript if the page does not have anoindex
directive on it. That means we only have to worry about SPA-renderedrobots
tags if the initial tag was notnoindex
.