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

Suggestion to the PR about add React and Vue components #39

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

shapkarin
Copy link
Contributor

@shapkarin shapkarin commented Dec 1, 2020

suggestions to the PR #38

@shapkarin shapkarin requested a review from davidyuk December 1, 2020 17:53
@shapkarin shapkarin force-pushed the feature/vue-and-react-versions branch from 43622c0 to 0c49ed9 Compare December 1, 2020 17:54
@shapkarin shapkarin force-pushed the feature/suggestions_to_pr_38 branch from eea8567 to e91981a Compare December 1, 2020 18:01
@shapkarin shapkarin force-pushed the feature/suggestions_to_pr_38 branch from e91981a to c87d8bc Compare December 1, 2020 18:02
Copy link
Member

@davidyuk davidyuk left a comment

Choose a reason for hiding this comment

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

Let's merge #38 firstly

@@ -26,6 +25,20 @@ version for React or Vue by importing `dist/react-without-styles.js` or
`dist/vue-without-styles.js` accordingly. The framework-specific version contains
all features available in the default one plus specific for particular framework wrappers.

### Develop
Copy link
Member

Choose a reason for hiding this comment

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

I agree that this was not covered well, but have you noticed a similar section at the end of readme? I'm proposing to update it instead in a2aa925

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I saw that later after this commit

display: -ms-flexbox;
display: -webkit-flex;
display: flex;
-webkit-flex-direction: column;
Copy link
Member

Choose a reason for hiding this comment

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

Don't vue cli has autoprefixer enabled by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will remove prefixes

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that we need prefixes in sites for developers at all.

@@ -15,3 +20,30 @@ export default {
},
}
</script>

<style>
.Example-List {
Copy link
Member

Choose a reason for hiding this comment

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

These styles are duplicated, doesn't look good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, needs to be combined to keep all examples the same

<SuperheroButton size="medium" account="ak_... or .chain name" />

{['large', 'small', 'icon'].map(size =>
Copy link
Member

Choose a reason for hiding this comment

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

I think the initial example is fine, and we shouldn't spend any more time working on this. The purpose of building these examples was to ensure that our button is compatible with different use cases.

Copy link
Contributor Author

@shapkarin shapkarin Dec 2, 2020

Choose a reason for hiding this comment

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

I think that until we don't have any tests, it will be useful to render as much possible variances as we can, to make it easier to check if something is wrong.

Copy link
Member

Choose a reason for hiding this comment

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

Let's combine it with something useful I'm proposing to build a code generator previewing the current state (#18). But before doing that you need to ensure that you have approval for that. cc: @mradkov

@shapkarin
Copy link
Contributor Author

Let's merge #38 firstly

@davidyuk agree

Base automatically changed from feature/vue-and-react-versions to master December 2, 2020 10:19
@davidyuk davidyuk force-pushed the master branch 5 times, most recently from 22c4bb6 to f1e1731 Compare December 3, 2020 03:58
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.

2 participants