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

fix(script): script info tab overflow #145

Merged
merged 6 commits into from
Dec 1, 2023
Merged

Conversation

Daryl-L
Copy link

@Daryl-L Daryl-L commented Nov 12, 2023

Copy link

vercel bot commented Nov 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ckb-explorer-frontend-in-magickbase-repo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 30, 2023 8:20am

src/index.css Outdated
}

*[role]:focus {
outline: none;
}

/* stylelint-disable-next-line selector-class-pattern */
.ant-tabs-tab.ant-tabs-tab-active {
Copy link
Member

Choose a reason for hiding this comment

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

Is this issue fixed by moving the overridden to the global context?

Choose a reason for hiding this comment

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

Is this issue fixed by moving the overridden to the global context?

It may have been fixed using the recommended and more standardized approach by antd, using the TabPane component.

The class name here is likely due to the original CSS module being converted to a styled-components implementation and not having a suitable place to put it, so it was moved here.

However, it is not quite appropriate to have it here. This is more like a theme customization. I suggest avoiding global theme overrides as much as possible and keeping them limited to encapsulated components, such as within ScriptTab.

Copy link
Author

Choose a reason for hiding this comment

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

I put this here because I saw other ant style are defined here or other file as global.
Maybe I think there should be another PR to refactor these.

Choose a reason for hiding this comment

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

I put this here because I saw other ant style are defined here or other file as global.

It was originally .scriptTabs :global { .ant-tabs-tab-active { ... } }, so it is a local change that should only affect components under .scriptTabs. It should not be considered as a global change.

Maybe I think there should be another PR to refactor these.

If there are some common theme changes, they can be encapsulated into themed Ant Design components.

However, I'm not sure if this situation occurs frequently enough to warrant it.

Copy link
Author

Choose a reason for hiding this comment

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

I put this here because I saw other ant style are defined here or other file as global.

It was originally .scriptTabs :global { .ant-tabs-tab-active { ... } }, so it is a local change that should only affect components under .scriptTabs. It should not be considered as a global change.

Maybe I think there should be another PR to refactor these.

If there are some common theme changes, they can be encapsulated into themed Ant Design components.

However, I'm not sure if this situation occurs frequently enough to warrant it.

Not only components under .scriptTabs changed, I tried it, so I just put them here

Choose a reason for hiding this comment

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

Not only components under .scriptTabs changed, I tried it, so I just put them here

You can refer to the compiled CSS result.

Source code

.scriptTabs {
  display: flex;
  flex-direction: column;
  justify-content: center;

  :global {
    .ant-tabs-nav {
      &::before {
        border-bottom-width: 4px;
      }
    }

    .ant-tabs-nav-list {
      padding-left: 40px;
    }

    .ant-tabs-tab-btn {
      color: #333;
      font-weight: 400;
      font-size: 20px;
      line-height: 23px;
      margin-bottom: 0;

      &[aria-selected='true'] {
        font-weight: 500;
      }
    }
  }
}

Build result

.styles_scriptTabs__K3ITv {
  display: flex;
  flex-direction: column;
  justify-content: center;
}
.styles_scriptTabs__K3ITv .ant-tabs-nav::before {
  border-bottom-width: 4px;
}
.styles_scriptTabs__K3ITv .ant-tabs-nav-list {
  padding-left: 40px;
}
.styles_scriptTabs__K3ITv .ant-tabs-tab-btn {
  color: #333;
  font-weight: 400;
  font-size: 20px;
  line-height: 23px;
  margin-bottom: 0;
}
.styles_scriptTabs__K3ITv .ant-tabs-tab-btn[aria-selected=true] {
  font-weight: 500;
}

Copy link
Author

Choose a reason for hiding this comment

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

I just put them to the specific components

@FrederLu
Copy link

Does this pr still need to be adjusted?

@Daryl-L
Copy link
Author

Daryl-L commented Nov 24, 2023

Does this pr still need to be adjusted?

Yes

@FrederLu FrederLu self-requested a review November 30, 2023 02:12
@FrederLu
Copy link

FrederLu commented Nov 30, 2023

2023-11-30.10.18.32.mov
  1. When calling the cell info pop-up window and changing the resolution, the pop-up window will automatically close.
  2. When the ··· button appears in the pop-up window and the mouse is hovering to select information, the pop-up window will also be closed.

These two problems should be abnormal. Need to confirm.

@FrederLu
Copy link

FrederLu commented Dec 1, 2023

1、When calling the cell info pop-up window and changing the resolution, the pop-up window will automatically close.

Is this updated?

@Keith-CY Keith-CY added this pull request to the merge queue Dec 1, 2023
Merged via the queue into develop with commit a44f6e3 Dec 1, 2023
6 checks passed
@Keith-CY Keith-CY deleted the fix/script-info-css branch December 1, 2023 09:56
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.

4 participants