Skip to content

chore: Adjust language picker theme to match other menus#14644

Merged
rusackas merged 1 commit into
apache:masterfrom
michael-s-molina:adjust-language-picker-theme
May 17, 2021
Merged

chore: Adjust language picker theme to match other menus#14644
rusackas merged 1 commit into
apache:masterfrom
michael-s-molina:adjust-language-picker-theme

Conversation

@michael-s-molina
Copy link
Copy Markdown
Member

@michael-s-molina michael-s-molina commented May 14, 2021 •
edited
Loading

SUMMARY

Adjusts language picker theme to match other menus.

Follow up of #14557

@rusackas

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

118169730-a8812380-b3ff-11eb-9565-27518efc0e99

Screen Shot 2021-05-14 at 12 07 38 PM

TEST PLAN

1 - Enable multiple languages
2 - Check language picker

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@codecov
Copy link
Copy Markdown

codecov Bot commented May 14, 2021

Codecov Report

Merging #14644 (3e6a470) into master (97c9e37) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #14644   +/-   ##
=======================================
  Coverage   77.46%   77.46%           
=======================================
  Files         958      958           
  Lines       48478    48482    +4     
  Branches     5681     5691   +10     
=======================================
+ Hits        37553    37557    +4     
  Misses      10725    10725           
  Partials      200      200
Flag Coverage Δ
javascript 72.53% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...et-frontend/src/components/Menu/LanguagePicker.tsx 100.00% <100.00%> (+15.00%) ⬆️
...uperset-frontend/src/components/Menu/MenuRight.tsx 93.18% <100.00%> (ø)
...tend/src/explore/components/ExploreChartHeader.jsx 62.50% <0.00%> (-0.66%) ⬇️
...ntend/src/explore/components/ExploreChartPanel.jsx 17.14% <0.00%> (ø)
...nd/src/explore/components/ExploreViewContainer.jsx 2.33% <0.00%> (ø)
...rontend/src/views/CRUD/dashboard/DashboardList.tsx 76.00% <0.00%> (+0.16%) ⬆️
...perset-frontend/src/views/CRUD/chart/ChartList.tsx 74.10% <0.00%> (+0.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97c9e37...3e6a470. Read the comment docs.

@junlincc junlincc requested review from pkdotson and rusackas May 14, 2021 20:59
@junlincc junlincc added the bash! label May 14, 2021
@michael-s-molina michael-s-molina mentioned this pull request May 17, 2021
Closed
geido
geido reviewed May 17, 2021
View reviewed changes
Comment thread superset-frontend/src/components/Menu/LanguagePicker.tsx
import React, { useState } from 'react';
import { Select } from 'src/common/components';
import { styled, useTheme } from '@superset-ui/core';
import Icons from 'src/components/Icons';
Copy link
Copy Markdown
Member

@geido geido May 17, 2021

Choose a reason for hiding this comment

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

Is there a reason why you are reverting back to the older Icon implementation?

Copy link
Copy Markdown
Member Author

@michael-s-molina michael-s-molina May 17, 2021

Choose a reason for hiding this comment

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

Yes. All menus are using the old implementation. If I change to the new one just here, we have differences in icons sizes. When we migrate the menu icons, then we should also migrate the icon in LanguagePicker.

pkdotson
pkdotson approved these changes May 17, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@pkdotson pkdotson left a comment

Choose a reason for hiding this comment

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

LGTM! I think the approach for icons should be fine for now.

@rusackas rusackas merged commit 4e6169e into apache:master May 17, 2021
@junlincc junlincc added rush! Requires immediate attention and removed rush! Requires immediate attention labels May 18, 2021
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
@michael-s-molina
chore: Adjust language picker theme to match other menus (apache#14644)
50e9849
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
@michael-s-molina
chore: Adjust language picker theme to match other menus (apache#14644)
a9e86d3
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@michael-s-molina
chore: Adjust language picker theme to match other menus (apache#14644)
a937c19
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 First shipped in 1.3.0 labels Mar 12, 2024
qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026
@michael-s-molina
chore: Adjust language picker theme to match other menus (apache#14644)
277f086
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@geido geido geido left review comments

@pkdotson pkdotson pkdotson approved these changes

@rusackas rusackas Awaiting requested review from rusackas

Assignees

No one assigned

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 1.3.0 First shipped in 1.3.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@michael-s-molina @pkdotson @geido @mistercrunch @rusackas @junlincc