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

Feature/refactor address format stylesheet #3

Merged

Conversation

eternalharvest
Copy link
Contributor

I refactor the style sheet related to the address format.
Just look these images...

BEFORE

before

AFTER

after
From now on, Japanese zip code symbol "〒" is not shown in editing mode (readonly).
It seems the height of drop-down list (state) is shorter than other fields, but i think this is another issue of odoo style sheet.

@eternalharvest eternalharvest force-pushed the feature/refactor_address_format_stylesheet branch 2 times, most recently from 00278dc to 8bb6697 Compare April 2, 2018 01:44
@yostashiro
Copy link
Sponsor Member

@eternalharvest Thanks a lot for your PR! It looks like a very nice adjustment. We will review this shortly. (I only noticed this PR now. I somehow didn't get an email.)

@@ -0,0 +1,16 @@
# EditorConfig helps developers define and maintain consistent

Choose a reason for hiding this comment

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

Do we need this file?

Copy link
Contributor Author

@eternalharvest eternalharvest Apr 10, 2018

Choose a reason for hiding this comment

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

@TimLai125

I'm using vim and the default indent of vim is tab.
So, I need to take care about indent size when I edit code...

editorconfig is not only for vim editor.
Almost all modern IDE support this file within itself or some plugin.
I think editorconfig helps any developers work.

But If you don't want to include this file within this repo,
I'll rebase to remove this patch from change-sets of this PR.

Anyway, thank u for reviewing my PR.

FYI: As u may know what is the editorconfig already, please refere to the following link.
http://editorconfig.org/

Copy link
Sponsor Member

@yostashiro yostashiro Apr 11, 2018

Choose a reason for hiding this comment

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

@eternalharvest Could you please remove .editorconfig file since having something for IDE/editor is outside the scope of this repository. No need to rebase this time although it's also fine if you do so.

Copy link
Sponsor Member

@yostashiro yostashiro left a comment

Choose a reason for hiding this comment

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

@eternalharvest Thank you very much for the PR. I'd suggest you add yourself to the list of contributors in README.rst.

Another thing - could you please sign OCA CLA https://odoo-community.org/page/cla for future contributions? I think it is not needed this time since we do not have any machine code in this module. Thank you!

<?xml version="1.0" encoding="utf-8"?>
<odoo>
<data noupdate="0">
<template id="assets_common" inherit_id="web.assets_backend">
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

It'd be more appropriate to name template id assets_backend instead of assets_common.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, It's my typo!
I fixed the template id.

And I removed the editorconfig file and added my self to the list of contribution.
I'll try to sign OCA CLA.

@eternalharvest eternalharvest force-pushed the feature/refactor_address_format_stylesheet branch 2 times, most recently from 6f4cccc to 06ecdab Compare April 11, 2018 04:48
@eternalharvest eternalharvest force-pushed the feature/refactor_address_format_stylesheet branch from 06ecdab to 1636367 Compare April 11, 2018 04:51
Copy link

@TimLai125 TimLai125 left a comment

Choose a reason for hiding this comment

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

LGTM

@yostashiro yostashiro merged commit 40e686f into OCA:11.0 Apr 11, 2018
@yostashiro
Copy link
Sponsor Member

@eternalharvest Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants