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

Feature: Replace followed threads links with my posts link #12

Merged

Conversation

LemonIllusion
Copy link
Contributor

No description provided.

@LemonIllusion
Copy link
Contributor Author

The operation fails and an error is logged if user is not logged in. Is it possible to implement isLoggedIn in a reasonable way or should the error just be caught and suppressed?

@LemonIllusion LemonIllusion force-pushed the feature/replace-followed-button branch 2 times, most recently from 56c5dcc to 12152fa Compare December 22, 2018 23:08
Copy link
Owner

@SimonAlling SimonAlling left a comment

Choose a reason for hiding this comment

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

Nice feature! I would like to request some changes:

  • The icon doesn't really fit in to me due to its white part, and it usually means "unread". What do you think about the one with two stacked papers (-250px -100px) instead?
  • I think italics would look nicer than quotation marks in the preference label – i.e. `Ersätt knappen <em>Följda trådar</em> med <em>Mina inlägg</em>`. It also lends itself better to styling.
  • Rename REPLACE_FOLLOWED_BUTTON to REPLACE_FOLLOWED_THREADS_LINK in operations.ts.
  • Rename the operation file correspondingly to replace-followed-threads-link.ts.
  • followed threads link instead of followed threads button in the operation description.
  • textContent instead of innerHTML in the operation.
  • SITE.CLASS.hasUnread (and create it) instead of "hasUnread" in the operation.
  • replace_followed_threads_link instead of replace_followed_button in the preference (and wherever it is used).
  • Rename the SASS file to replace-followed-threads-link.scss.

Also, due to the commit message limit of 50 characters that I use and in accordance with my requested changes of nomenclature, I suggest this message instead:

Replace followed threads link with my posts link

@SimonAlling
Copy link
Owner

Regarding isLoggedIn: The other detectors in the environment module depend only on data available at document-start, namely document.location.pathname. For isLoggedIn to make sense, I think it should be implemented in the same way, i.e. not by looking at any page content.

And I hereby proudly present a solution:

export function isLoggedIn(): boolean {
    return (document.head.textContent as string).includes(`'visitorType': 'member'`);
}

I have pushed the implementation above, so you should be able to use it if you pull that commit (d286580).

@LemonIllusion LemonIllusion changed the title Feature: Replace followed threads button with my posts link Feature: Replace followed threads links with my posts link Dec 23, 2018
@LemonIllusion LemonIllusion force-pushed the feature/replace-followed-button branch 2 times, most recently from af809ab to fc50633 Compare December 23, 2018 01:27
LemonIllusion and others added 3 commits December 23, 2018 02:44
Detectors in the environment module are intended to work at
document-start, so they cannot look for anything in <body>. The
implementation in this commit relies on this snippet in <head>:

    // <![CDATA[
    dataLayer = [{
      // 'userID': comming-soon,
      'visitorType': 'member'
    }];
    // ]]>

(If the user is not logged in, 'visitorType' is 'guest' instead.)
@LemonIllusion
Copy link
Contributor Author

LemonIllusion commented Dec 23, 2018

I think I've gitted far beyond my proper understanding, but that should be every change you requested and then some. Anything more to do?

Copy link
Owner

@SimonAlling SimonAlling left a comment

Choose a reason for hiding this comment

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

I found three more things I would like to change. Two of them I should have seen the first time, but here goes:

  • The preference label should say länken instead of knappen.
  • The preference should be moved up so it sits between insert_preferences_shortcut and insert_web_search_button. The reason is that insert_web_search_button and search_engine should be adjacent to each other. Note that this needs to be changed in the text module as well, for consistency.
  • In the operation, HTMLLinkElement is actually not correct; it should be HTMLAnchorElement. Classic mistake that I've made a couple of times myself. :)

I think you can simply make these changes in a new commit; I'll squash all the commits in this feature branch into a single one anyway.

@LemonIllusion
Copy link
Contributor Author

Do you like the idea of using ${general.my_posts} instead of writing out Mina inlägg in the setting label? It's less repetition, but would force a specific ordering of the text groups and possibly cause issues with circular dependencies if encouraged.

@SimonAlling
Copy link
Owner

Do you like the idea of using ${general.my_posts} instead of writing out Mina inlägg in the setting label? It's less repetition, but would force a specific ordering of the text groups and possibly cause issues with circular dependencies if encouraged.

Yes, I do. I think the best way is to add an internal (non-exported) string

const my_posts = "Mina inlägg";

above export const general = { (with an empty line in between) and then use it for both general.my_posts and preferences.general.replace_followed_threads_link. In general, you can just do

my_posts,

rather than

my_posts: my_posts,

since the names are identical.

@SimonAlling SimonAlling merged commit 7122061 into SimonAlling:develop Dec 23, 2018
@LemonIllusion LemonIllusion deleted the feature/replace-followed-button branch December 23, 2018 12:48
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.

None yet

2 participants