Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Cleanups #3742

Merged
merged 12 commits into from
Dec 9, 2016
Merged

Cleanups #3742

merged 12 commits into from
Dec 9, 2016

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Dec 8, 2016

  • Use new ~ URLs where applicable/missed
  • Removed unused files
  • Move files to correct locations
  • Combine similar functionality into one file, multiple exports
  • Remove unused styles

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Dec 8, 2016
@@ -15,17 +15,6 @@
// along with Parity. If not, see <http://www.gnu.org/licenses/>.

import { stringify } from 'querystring';
import React from 'react';

export const termsOfService = (
Copy link
Contributor

Choose a reason for hiding this comment

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

@jacogr

I put this here on purpose. The terms of service describe the legal implications of using the 3rd party service in 3rdparty/sms-verification. I can understand that having React in here feels weird, but since termsOfService is stateless & static, I think it's worth it. Having it in modals/SMSVerification/GatherData is very unintuitive imho.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very weird indeed, since the 3rdparty libraries are also stuff that we would like to start publishing under the @parity/ namespace. Unless we make an exception for the specific one and don't publish it at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will revert that one, but breaks the purpose or the area. (Will have to treat it as "special and internal only")

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we have three choices.

  1. not bundling the legal text with the library
  2. using sth like dangerouslySetInnerHTML.
  3. using plain text legal terms. they look less nice and the links are gone, but we still get rid of React.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suppose on #3, we could go with something like markdown - will actually kill 2 birds with one stone -

  • be readable as text
  • be transformable into pretty HTML

Copy link
Contributor

Choose a reason for hiding this comment

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

Markdown should get compiled on build. What about the template system currently in place? One you combine it with dangerouslySetInnerHTML.

@derhuerst
Copy link
Contributor

Disagree with moving of termsOfService. Otherwise 👍 👍

@derhuerst derhuerst added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 8, 2016
@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Dec 8, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 85.877% when pulling 8f81b10 on jg-housekeeping into f9a24f3 on master.

@derhuerst derhuerst added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 9, 2016
@jacogr jacogr merged commit befcc9c into master Dec 9, 2016
@jacogr jacogr deleted the jg-housekeeping branch December 9, 2016 12:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants