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

feat: Adjust panels spacing #448

Merged
merged 26 commits into from
Aug 16, 2023
Merged

feat: Adjust panels spacing #448

merged 26 commits into from
Aug 16, 2023

Conversation

janmichek
Copy link
Collaborator

Description

fixes #394
fixes #23

Demo

Checklist:

  • I have read and followed the Contributing Guide
  • abstract panel components
  • unify blank panels spacing
  • adjust panel spacing by visual material

@janmichek janmichek mentioned this pull request Aug 15, 2023
4 tasks
@janmichek janmichek changed the title Adjust panels spacing feat: Adjust panels spacing Aug 15, 2023
@github-actions
Copy link

Deployed to https://pr-448-aescan.stg.aepps.com

@janmichek
Copy link
Collaborator Author

Followup, I didn't dare because of many changes #449

class="account-details-panel__copy-chip-ellipse"/>
</div>
</header>
<template #heading>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

header styling was moved to AppPanel to release styles duplication

@@ -142,33 +138,26 @@ const sanitizedPrice = computed(() =>

<style scoped>
.account-details-panel {
padding: var(--space-4) var(--space-1) var(--space-3);
margin-bottom: var(--space-7);
&__table-header {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Styles were sorted and ordered, which uncover a lot of dead code

@@ -40,7 +40,7 @@ defineProps({
@media (--desktop) {
height: 32px;
font-size: 14px;
padding: 6px var(--space-1);
padding: var(--space-0) var(--space-1);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated minor chips tyles

@@ -83,13 +83,17 @@ const selectedValue = useVModel(props, 'modelValue', emit)
&__tags {
border-radius: 8px;
border-color: var(--color-midnight);
border-width: 1.5px;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated minor select tyles

@@ -73,9 +73,10 @@ function selectTab(tabIndex) {
cursor: pointer;
padding: var(--space-1);
background: var(--color-midnight-15);
color: var(--color-gray);
color: var(--color-midnight-55);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated colors of tabs

@@ -2,7 +2,7 @@
<app-panel class="auctions-panel">
<panel-header
level="h3"
title="Auctions ending soon"
title="AUCTIONS ENDING SOON"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reverted last changes in dashboard headings after consultation

@janmichek janmichek requested review from lukeromanowicz and michele-franchi and removed request for lukeromanowicz August 15, 2023 17:32
Copy link
Collaborator

@michele-franchi michele-franchi left a comment

Choose a reason for hiding this comment

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

Impressive work, really well done! 👏

I left some comments, could you have a look at them?

I personally don't like 1.5px borders and I don't understand where these come from, I checked Figma and it's always 1px. I didn't comment in the code because there are a lot of places but personally I would keep 1px everywhere, as It was before.

src/components/DashboardStateChannelsPanel.vue Outdated Show resolved Hide resolved
src/components/StatsTile.vue Outdated Show resolved Hide resolved
src/components/TransactionCellNameRevokeTx.vue Outdated Show resolved Hide resolved
src/pages/keyblocks/[id].vue Outdated Show resolved Hide resolved
src/pages/oracles/index.vue Outdated Show resolved Hide resolved
src/pages/microblocks/[id].vue Outdated Show resolved Hide resolved
src/pages/tokens/index.vue Outdated Show resolved Hide resolved
@janmichek
Copy link
Collaborator Author

Impressive work, really well done! 👏

I left some comments, could you have a look at them?

I personally don't like 1.5px borders and I don't understand where these come from, I checked Figma and it's always 1px. I didn't comment in the code because there are a lot of places but personally I would keep 1px everywhere, as It was before.

Well, this is tricky. In code it is 1px, but in every zoomed scale looks like 1.5px,
It's surprising to me that such a mature tool as Figma cannot render correctly. I have been working with Figma several times before and I didn't face such render issues. Can you provide some feedback @danilosierrac ?

@michele-franchi
Copy link
Collaborator

Impressive work, really well done! 👏
I left some comments, could you have a look at them?
I personally don't like 1.5px borders and I don't understand where these come from, I checked Figma and it's always 1px. I didn't comment in the code because there are a lot of places but personally I would keep 1px everywhere, as It was before.

Well, this is tricky. In code it is 1px, but in every zoomed scale looks like 1.5px, It's surprising to me that such a mature tool as Figma cannot render correctly. I have been working with Figma several times before and I didn't face such render issues. Can you provide some feedback @danilosierrac ?

It's how Figma works and the goal is to have everything looks the same on different screens so a design looks the same for everyone but at the end I think the value that should be used is the one defined in the code / properties section.

Other than how it looks on Figma, on aeScan 1px looks a bit softer than 1.5px and I personally prefer it.

@janmichek
Copy link
Collaborator Author

@michele-franchi I don't think that is how Figma works. I didn't have that issue before. However, I fixed it. All borders aligned to 1px

@michele-franchi
Copy link
Collaborator

@janmichek for example for fonts:
https://forum.figma.com/t/why-does-a-font-weight-in-figma-seem-lighter-than-the-same-weight-in-the-browser/2207

Figma uses its own font rendering because each device renders fonts differently depending on the device, OS type and version, browser and its version, system settings, browser settings, display and maybe some other factors. Figma has to use one font rendering for all because it’s a collaborative tool with one source of truth.

Copy link
Collaborator

@michele-franchi michele-franchi left a comment

Choose a reason for hiding this comment

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

Very good job 🚀

Copy link
Contributor

@lukeromanowicz lukeromanowicz left a comment

Choose a reason for hiding this comment

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

First of all, that's quite an amazing job you've done. After recent fixes I didn't find anything major to improve. The only thing I'm not very happy about some weird paddings like 7px or sub px values, but I suppose it's according to the designs. Well done, mate!

padding: var(--space-2) 0 var(--space-1);
line-height: 18px;
vertical-align: top;
padding: 10px 0 8px;
Copy link
Contributor

Choose a reason for hiding this comment

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

We could use --space-1 here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

vertical-align: middle;

@media (--desktop) {
padding: var(--space-1) var(--space-0);
padding: 10px var(--space-0) 8px;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use --space-1 here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yea, I missed that. Fixed

@janmichek janmichek requested review from Liubov-crypto and removed request for Liubov-crypto August 16, 2023 12:43
@janmichek
Copy link
Collaborator Author

@Liubov-crypto Could you please go through the whole app and check if anything is obviously broken from UI side. Please don't compare to design material, as there are more misalignments. Thank you

Copy link
Collaborator

@Liubov-crypto Liubov-crypto left a comment

Choose a reason for hiding this comment

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

I found that the info is duplicated on Tokens details page, but I think this is middleware issue:
dubb

The rest, LGTM.

@janmichek
Copy link
Collaborator Author

I found that the info is duplicated on Tokens details page, but I think this is middleware issue: dubb

The rest, LGTM.

Rigth, that's a know thing. Thanks for testing

@janmichek janmichek merged commit e0ef32b into develop Aug 16, 2023
2 checks passed
@janmichek janmichek deleted the Adjust-panels-spacing branch August 16, 2023 15:07
@Liubov-crypto
Copy link
Collaborator

I found that the info is duplicated on Tokens details page, but I think this is middleware issue: dubb
The rest, LGTM.

Rigth, that's a know thing. Thanks for testing

You are welcome! :)

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.

Adjust panels spacing Unify blank states whitespacing
4 participants