Skip to content

Tiny graphical edit on in instrument tab#13

Closed
FredericGuilbault wants to merge 3 commits into
vokkim:masterfrom
FredericGuilbault:master
Closed

Tiny graphical edit on in instrument tab#13
FredericGuilbault wants to merge 3 commits into
vokkim:masterfrom
FredericGuilbault:master

Conversation

@FredericGuilbault
Copy link
Copy Markdown

Instrument data touching the right side of the window feels broken.
Like if some other text might be hidden.
putting a space between last character and the edge of the window remove confusion.

before/after:
https://imgur.com/a/WhLYb

@FredericGuilbault
Copy link
Copy Markdown
Author

Oh, would you prefer PR in a separate branch ?

Comment thread src/styles/app.less
font-size: 110%;
color: @labelFontColor;
text-align: right;
text-align: center;
Copy link
Copy Markdown
Owner

@vokkim vokkim Apr 10, 2018

Choose a reason for hiding this comment

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

Setting text-align: center will make the text "jump" when text length/value changes. I'd still prefer aligning the text on right, but maybe adding some padding or margin to the right.

Copy link
Copy Markdown
Author

@FredericGuilbault FredericGuilbault Apr 11, 2018

Choose a reason for hiding this comment

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

Fair point.
See better approach on PR #14

Comment thread package.json
"eslint-plugin-es5": "1.2.0",
"eslint-plugin-react": "7.7.0",
"express": "4.16.3",
"@geo-maps/countries-coastline-10km": "0.6.0",
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Is the countries-coastline-10km package necessary if the actual JSON file has been copied under world-base.geo.json? Maybe we could just add credits to the readme.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I would go the other way.
For more fluid maintenance in time. The JSON should be copied automatically on build or used strait from it's original location.

Im not much used to nodejs but this seem the way to go. Although I woudn't know what would be the best strategy. So I did not push anything on this part exept adding the source package.

It's an improvement but an incomplete feature. Maybe it should come with the opening of an issue.

Copy link
Copy Markdown
Author

@FredericGuilbault FredericGuilbault Apr 17, 2018

Choose a reason for hiding this comment

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

@vokkim What would be the best thing to do with this ?

Im keeping it in my fork untill it's a full feature.

You accept it like that and I open an issue about it on your repo.

Or I remove the dependency, add credits to the readme and you will accept it.

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