Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

GH-7680 Migrate get post link modal to be pure #234

Merged

Conversation

h2oloopan
Copy link
Contributor

Summary

Migrated get post link modal to be a pure component and use Redux.
No store or action is involved

Ticket Link

mattermost/mattermost#7680

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • Added or updated unit tests (required for all new features)

@h2oloopan
Copy link
Contributor Author

Hi, this is my first try to work on the webapp, let me know if there is any problem.

@jasonblais jasonblais added the 2: Dev Review Requires review by a core commiter label Nov 3, 2017
@jasonblais
Copy link
Contributor

Great, thanks @h2oloopan! Looks like the build fails, can you take a look at it?

If you're not sure why, let us know and we can help out

@h2oloopan
Copy link
Contributor Author

Hmm, I can't check details on GitHub. I will see if the build problem occurs on my local machine when I get home. On the other hand, can you help me post the error log from the build server here so I can take a look at it first? Thanks!

@jwilander jwilander closed this Nov 3, 2017
@jwilander jwilander reopened this Nov 3, 2017
@jwilander
Copy link
Member

Looks like the build suddenly stopped halfway through for no reason. Re-running it

@jwilander
Copy link
Member

You've got this style error:

/home/jenkins/workspace/mattermost-webapp_PR-234-J6QHJH4SKMQXQUAKF2R23SKREFOKMBK3XQBFJUEEPGJ32M5RXJZQ/mattermost-webapp/tests/components/get_post_link_modal/get_post_link_modal.test.jsx
  30:7  error  Missing semicolon  semi

✖ 1 problem (1 error, 0 warnings)

@h2oloopan
Copy link
Contributor Author

Was travelling on the weekend. Finally got back home and setup eslint on my local machine. The semicolons should have been fixed now.

@saturninoabril
Copy link
Member

@h2oloopan Thank you for your contribution!
I hope it's fine with you, I added component test to your PRs.

Copy link
Member

@saturninoabril saturninoabril left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@saturninoabril saturninoabril added this to the v4.5.0 milestone Nov 7, 2017
@h2oloopan
Copy link
Contributor Author

@saturninoabril Thanks for the added test!

@jwilander jwilander added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Nov 7, 2017
@jwilander jwilander merged commit 475236a into mattermost:master Nov 7, 2017
@jwilander
Copy link
Member

Thanks for this @h2oloopan !

@lindalumitchell lindalumitchell added the Tests/Not Needed Does not require new release tests label Nov 14, 2017
@jasonblais jasonblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Jan 4, 2018
hmhealey pushed a commit that referenced this pull request Aug 28, 2020
* only set fetch's credentials:include option if no access token

this is what client.js does, and fixes a CORS issue when running client4.js in a browser.
Specifically, the issue seen is:

Fetch API cannot load http://127.0.0.1:8065/api/v4/users/me. Response to preflight request doesn't pass access control
check: The value of the 'Access-Control-Allow-Credentials' header in the response is '' which must be 'true' when the
request's credentials mode is 'include'. Origin 'http://127.0.0.1:8002' is therefore not allowed access.

* Revert "only set fetch's credentials:include option if no access token"

This reverts commit 2ba1306a69e46f84a7143c2ce2753a1f222cb118.

* make sending credentials via fetch configurable in client4.js

this preserves the default behaviour of sending cookies as part of the request, but can
be turned off with client.setIncludeCookes(false).
hmhealey pushed a commit that referenced this pull request Mar 17, 2021
* only set fetch's credentials:include option if no access token

this is what client.js does, and fixes a CORS issue when running client4.js in a browser.
Specifically, the issue seen is:

Fetch API cannot load http://127.0.0.1:8065/api/v4/users/me. Response to preflight request doesn't pass access control
check: The value of the 'Access-Control-Allow-Credentials' header in the response is '' which must be 'true' when the
request's credentials mode is 'include'. Origin 'http://127.0.0.1:8002' is therefore not allowed access.

* Revert "only set fetch's credentials:include option if no access token"

This reverts commit 2ba1306a69e46f84a7143c2ce2753a1f222cb118.

* make sending credentials via fetch configurable in client4.js

this preserves the default behaviour of sending cookies as part of the request, but can
be turned off with client.setIncludeCookes(false).
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation Tests/Not Needed Does not require new release tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants