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

FXD-1214 fix(css): css refactor 4/4 #330

Merged
merged 10 commits into from
Aug 9, 2018
Merged

FXD-1214 fix(css): css refactor 4/4 #330

merged 10 commits into from
Aug 9, 2018

Conversation

qingyashu
Copy link
Contributor

No description provided.

@qingyashu qingyashu force-pushed the fix/css-refactor-4 branch 2 times, most recently from 8a1e1f4 to f844345 Compare August 7, 2018 16:33
@qingyashu qingyashu closed this Aug 7, 2018
@qingyashu qingyashu reopened this Aug 7, 2018
abgeorge7
abgeorge7 reviewed Aug 7, 2018
View reviewed changes
src/QueryNode/QueryNode.less Outdated
@@ -0,0 +1,52 @@
.query-node__search-button {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this class or the other query-node__search-button classes used anywhere?

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 think git is doing bad job at merging...previous code missing T.T

src/QueryNode/QueryNode.less Outdated
color: #2B547E;
}

.query-node__input {
Copy link
Contributor

Choose a reason for hiding this comment

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

same here and with query-node__select

src/QueryNode/QueryNode.jsx Outdated
@@ -1,36 +1,11 @@
import React from 'react';
import { Link } from 'react-router-dom';
import styled, { css } from 'styled-components';
import Select from 'react-select';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this import used?

src/components/CheckBox.less Outdated
border-bottom: 0;
}

.checkbox-label {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is also picky but can it be .checkbox__label? Just because the whole thing seems like a checkbox and then the label is just part of it... this also might be a crazy request feel free to ignore/argue with me 🤪

Copy link
Contributor Author

Choose a reason for hiding this comment

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

typo....

src/components/CheckBox.less Outdated
max-width: 12vw;
overflow: hidden;
padding: 0px 0.25rem;
margin-left: 3px 3px 3px 0.3rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is what the original css looked like but what is up with these mixed units 😂

src/components/Popup.less Outdated
height: 70px;
line-height: 70px;
padding: 0px 30px;
background: #3283c8;
Copy link
Contributor

Choose a reason for hiding this comment

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

@blue

src/components/Popup.less Outdated

.popup__message {
font-size: 1em;
background: mid_light_gray;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is a valid color

src/components/SelectComponent.less Outdated
}

.selection__title {
vertical-align: middle;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is redundant

src/components/SelectComponent.less Outdated

.selection__select {
display: inline-block;
vertical-align: middle;
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe same here?

src/components/buttons/IconicLink.jsx
@@ -37,6 +37,7 @@ IconicLink.propTypes = {
iconColor: PropTypes.string,
caption: PropTypes.string,
buttonClassName: PropTypes.string,
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need button class name and class name?

@qingyashu qingyashu force-pushed the fix/css-refactor-4 branch 2 times, most recently from 78ba3f2 to d3d6a43 Compare August 7, 2018 20:22
abgeorge7
abgeorge7 suggested changes Aug 7, 2018
View reviewed changes
Copy link
Contributor

@abgeorge7 abgeorge7 left a comment

Choose a reason for hiding this comment

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

I think the folder restructuring broke some Storybook links.

The select component for the old Explorer table (at the bottom) and the input for "browsing nodes" also looks off from master.

src/components/buttons/IconicLink.jsx Outdated
@@ -36,15 +36,17 @@ IconicLink.propTypes = {
icon: PropTypes.string,
iconColor: PropTypes.string,
caption: PropTypes.string,
buttonClassName: PropTypes.string,
orangeButton: PropTypes.bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want this to be a prop type because it's not very reusable... why not keep the original buttonClassName and pass it either 'button-primary-orange' or 'button-primary-white' ?

src/QueryNode/QueryForm.jsx
<Input placeholder='submitter_id' type='text' name='submitter_id' />
<SearchButton type='submit' onSubmit={this.handleQuerySubmit} value='search' />
<Select className='query-form__select' name='node_type' options={options} value={state.selectValue} onChange={this.updateValue} />
<input className='query-form__input' placeholder='submitter_id' type='text' name='submitter_id' />
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this styling is missing - the input field looks unstyled

@qingyashu
Copy link
Contributor Author

Fixed some style mismatch, remove styled component from theme.js, also change back to keep className and buttonClassName for IconicLink (These are for different tags, one for IconicLink one for button)

@qingyashu qingyashu changed the title fix(css): css refactor 4/4 FXD-1214 fix(css): css refactor 4/4 Aug 8, 2018
@qingyashu qingyashu merged commit de46a00 into master Aug 9, 2018
@qingyashu qingyashu deleted the fix/css-refactor-4 branch September 28, 2018 22:04
@cstephanos cstephanos mentioned this pull request Nov 4, 2022
Open
@urvi-occ urvi-occ mentioned this pull request Nov 4, 2022
Open
@pmartinov pmartinov mentioned this pull request Nov 5, 2022
Open
@cstephanos cstephanos mentioned this pull request Nov 5, 2022
Open
@urvi-occ urvi-occ mentioned this pull request Dec 25, 2022
Open
This was referenced Dec 25, 2022
Closed
Closed
@cstephanos cstephanos mentioned this pull request Dec 25, 2022
Open
@snyk-bot snyk-bot mentioned this pull request Dec 25, 2022
Open
@cstephanos cstephanos mentioned this pull request Dec 26, 2022
Open
@Mikisha10 Mikisha10 mentioned this pull request Dec 26, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abgeorge7 abgeorge7 abgeorge7 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@qingyashu @abgeorge7