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

x-follow-button #59

Merged
merged 36 commits into from
Dec 4, 2019
Merged

x-follow-button #59

merged 36 commits into from
Dec 4, 2019

Conversation

kateloschinina
Copy link
Contributor

@kateloschinina kateloschinina commented May 25, 2018 •
edited
Loading

Using concept name as button text:
screenshot 2019-02-06 at 16 29 09
screenshot 2019-02-06 at 16 29 03

Using generic button text:
screenshot 2019-02-06 at 16 28 49
screenshot 2019-02-06 at 16 28 55

Currently in use directly by next-myft-page and ft-app, and as a dependency of x-topic-search.

@sourcier sourcier self-assigned this May 25, 2018
apaleslimghost
apaleslimghost reviewed Jul 10, 2018
View reviewed changes
components/x-follow-button/src/Button.jsx Outdated Show resolved Hide resolved
components/x-follow-button/src/styles/main.scss Outdated Show resolved Hide resolved
components/x-follow-button/src/FollowButton.jsx Outdated Show resolved Hide resolved
components/x-follow-button/src/Button.jsx Outdated Show resolved Hide resolved
components/x-follow-button/src/FollowButton.jsx Outdated Show resolved Hide resolved
components/x-follow-button/src/Input.jsx Outdated Show resolved Hide resolved
components/x-follow-button/stories/index.js Outdated Show resolved Hide resolved
@apaleslimghost apaleslimghost mentioned this pull request Jul 13, 2018
Merged
@apaleslimghost
Copy link
Member

@kateloschinina i just merged master into this branch, and we've added ESLint which has a few errors on this branch, can you fix them please?

/home/travis/build/Financial-Times/x-dash/components/x-follow-button/src/FollowButton.jsx
   6:8   error  'styles' is defined but never used  no-unused-vars
   8:42  error  'props' is defined but never used   no-unused-vars
  11:3   error  Unexpected console statement        no-console
  12:3   error  Unexpected console statement        no-console
  13:3   error  Unexpected console statement        no-console
  15:16  error  'event' is defined but never used   no-unused-vars
/home/travis/build/Financial-Times/x-dash/components/x-follow-button/stories/knobs.js
  3:27  error  'object' is defined but never used  no-unused-vars
  3:41  error  'number' is defined but never used  no-unused-vars
  3:58  error  'date' is defined but never used    no-unused-vars
✖ 9 problems (9 errors, 0 warnings)

@i-like-robots i-like-robots added the component A new component in development label Jul 26, 2018
apaleslimghost
apaleslimghost previously approved these changes Jul 27, 2018
View reviewed changes
Copy link
Member

@apaleslimghost apaleslimghost left a comment

Choose a reason for hiding this comment

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

:shipit:

sorry for the delay!

Components automation moved this from Done to In development Nov 15, 2019
@kateloschinina
Add x-follow-button to the new story format
53c1f5a 
Post the update whilst rebasing to master
@kateloschinina kateloschinina temporarily deployed to x-dash-follow-button-vkjuewn6r November 15, 2019 15:52 Inactive
@kateloschinina kateloschinina mentioned this pull request Nov 20, 2019
Closed
@kateloschinina
Update snapshot
e9338da
@kateloschinina
Refactor x-follow-button storybook demo
21a1cc0 
 - Use regular story configuration
 - Clean code from the old-styled stories
@kateloschinina
Add tests
4b5843b 
 - Test conditional behaviour
 - Modify jest configuration to accomodate tests in the new
setup
@kateloschinina
Copy link
Contributor Author

Note: the build is currently failing as #415 is to be merged in first.

@edds
Use sass-loader in storybook webpack
4ae4119 
So that we can use Sass in components. This also adds all the bower
paths to the sass load path. Which works for the moment but might need
making more robust in the future.
@edds
Merge remote-tracking branch 'origin/master' into follow-button
04d7890
@edds
Update x-follow-button for origami major change
f7474c7 
So that we can include it in projects that use other components that use
the latest changes.
@edds
Copy link
Contributor

edds commented Dec 4, 2019

@kateloschinina I've gone ahead and pulled the changes from #415 into here but made webpack find the bower_component folders itself rather than using symlink which it can do at run time.

apaleslimghost
apaleslimghost reviewed Dec 4, 2019
View reviewed changes
.storybook/webpack.config.js
{
loader: require.resolve('sass-loader'),
options: {
includePaths: glob.sync('./components/*/bower_components', { absolute: true }),
Copy link
Member

Choose a reason for hiding this comment

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

nice solution

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit of a hack because if two different components depend on different versions of bower things then it could get sticky. but it gets this moving and we can iterate that later.

Copy link
Member

Choose a reason for hiding this comment

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

#183 should fix that in the future

apaleslimghost
apaleslimghost approved these changes Dec 4, 2019
View reviewed changes
Copy link
Member

@apaleslimghost apaleslimghost left a comment

Choose a reason for hiding this comment

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

i reckon this is good to go. it's used in production already so

@alicebartlett alicebartlett self-requested a review December 4, 2019 14:44
@edds edds merged commit 619d52e into master Dec 4, 2019
Components automation moved this from In development to Done Dec 4, 2019
@edds edds deleted the follow-button branch December 4, 2019 14:44
@kateloschinina
Copy link
Contributor Author

@kateloschinina I've gone ahead and pulled the changes from #415 into here but made webpack find the bower_component folders itself rather than using symlink which it can do at run time.

Thank you so much! 🌟

(can't believe it's merged - first commit of this PR made by me on 24 May 2018 😆)

@benbarnett
Copy link
Contributor

🎉 🎉 🎉

rowanbeentje pushed a commit that referenced this pull request Nov 24, 2021
@edds
Merge pull request #59 from Financial-Times/follow-button
615dd1d 
x-follow-button
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edds edds edds left review comments

@i-like-robots i-like-robots i-like-robots left review comments

@apaleslimghost apaleslimghost apaleslimghost approved these changes

@dan-searle dan-searle dan-searle left review comments

@fenglish fenglish fenglish left review comments

@GlynnPhillips GlynnPhillips Awaiting requested review from GlynnPhillips

@alicebartlett alicebartlett Awaiting requested review from alicebartlett

Assignees
No one assigned
Labels
component A new component in development FT Apps Apps specific ft.com Website specific
Projects
Components
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@kateloschinina @apaleslimghost @benbarnett @edds @i-like-robots @dan-searle @fenglish @sourcier